126 clippy by tcharding · Pull Request #285 · bitcoindevkit/bdk

tcharding · 2021-02-13T05:26:39Z

Description

Fix clippy warnings, this includes those found using nightly. This PR does not remove all clippy warnings. It doesn't do:

the ones done in @notmandatory's PR Fix clippy warnings for new stable rust 1.50.0 #282.
the float comparison ones in unit tests, I'm not sure about these ones yet - will think some more on them.
the all caps ones (e.g. BIP32, UTXO)
the Rust 2021 ones (are we even in line with 2018 yet :)
the 'use Default instead of new' one. Wasn't sure if that is a breaking API change or not so I left it out

I've tested the PR using a script that runs clippy for each of the toolchains +stable, +1.45, and +nightly to see none of the nightly suggestions break earlier toolchains. The final patch in this PR adds that script to scritps/cargo-check.sh. Can remove if not needed/wanted.

Notes to the reviewers

This PR includes patches first put forward in this PR, seems I cannot re-open that one, sorry for breaking the thread.

There are going to be merge conflicts with #282, I'm happy for it to go in first and I'll rebase this one.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

notmandatory

Overall looks good, thanks for taking the time to put clippy explanations in each commit, made it much easy to understand.

notmandatory · 2021-02-15T23:18:14Z

src/database/any.rs

+            AnyDatabase::Memory(db) => match batch {
+                AnyBatch::Memory(batch) => db.commit_batch(batch),
+                #[cfg(feature = "key-value-db")]
+                _ => unreachable!("Sled batch shouldn't be used with Memory db."),


Shouldn't this still be unimplemented! (with the same message)? since I believe this condition could be reached if batch is a Sled(db).

Sure thing, changed as suggested. I modified the original commit and force pushed. Is that the typical workflow here or do folks prefer not changing commits after review starts?

Thanks for the review!

A force push is fine, github does a good job of showing just changed files for anyone who's already started a review.

Sorry can you do another rebase, there aren't any conflicts but a couple new PRs just got merged. In particular the PR to changed the CI pipeline required checks from stable to 1.50.0 so that commit needs to be in your PR too.

No sweat, done.

src/database/any.rs

afilini · 2021-02-22T08:54:42Z

scripts/cargo-check.sh

+#!/bin/bash
+#
+# Run various invocations of cargo check
+


nit: I'd add a set -e to make the whole script fail as soon as there's an error

Nice, cheers!

afilini · 2021-02-22T10:20:50Z

src/wallet/signer.rs

+    // Return true if data field of `fat` pointer is the same address as `thin` pointer.
+    fn is_equal(fat: &Arc<dyn Signer>, thin: &Arc<DummySigner>) -> bool {
+        let (left, right) = raw_pointers(fat, thin);
+        left == right
+    }


I don't think this part is necessary, you should be able to just compare two signers using their .id().

I would change DummySigner to take a u64 inside, and then return it as its id with something like SignerId::Dummy(self.number).

Then if you create different signers with different values you can compare them using the id.

Cool, nice idea. One thing was a bit ugly, I had to use a helper function instead of implementing PartialEq since we only have a reference to a dyn Signer when comparing for equality. Please let me know if you see a cleaner way to do this.

afilini · 2021-02-22T10:21:30Z

src/wallet/tx_builder.rs

 mod test {
-    const ORDERING_TEST_TX: &'static str = "0200000003c26f3eb7932f7acddc5ddd26602b77e7516079b03090a16e2c2f54\
+    const ORDERING_TEST_TX: &str = "0200000003c26f3eb7932f7acddc5ddd26602b77e7516079b03090a16e2c2f54\
                                            85d1fd600f0100000000ffffffffc26f3eb7932f7acddc5ddd26602b77e75160\


nit: align the beginning of each line

My bad, thanks. Done.

afilini · 2021-02-22T10:22:43Z

I think it also needs one more rebase on master

tcharding · 2021-02-24T02:28:01Z

@afilini both your suggestions implemented as two new patches on this PR. Force push to do the rebase.

Thanks for the review.

As directed by clippy use `unwrap_or_else` in order to take advantage of lazy evaluation.

As directed by clippy use `!a.is_empty()` instead of `a.len() > 0`.

const str types do not need an explicit lifetime, remove it. Found by clippy.

No need to clone copy types, found by clippy.

Clippy emits warning: warning: needlessly taken reference of both operands Remove the explicit reference's as suggested.

Clippy emits warning: warning: avoid using `collect()` when not needed As suggested by clippy just use `count` directly on the iterator instead of `collect` followed by `len`.

As suggested by clippy we can use `.next()` on an iterator instead of `nth(0)`. Although arguably no clearer, and certainly no worse, it keeps clippy quiet and a clean lint is a good thing.

Clippy emits warning: warning: field assignment outside of initializer for an instance created with Default::default() Do as suggested by clippy and use the default init pattern. ``` let foo = Foo { bar: ..., Default::default() } ```

Clippy complains about use of a mutex, suggesting we use an `AtomicUsize`. While the same functionality _could_ be achieved using an `AtomicUsize` and a CAS loop it makes the code harder to reason about for little gain. Lets just quieten clippy with an allow attribute and document why we did so.

The `ChunksIterator` constructor is only used when either `electrum` or `esplora` features are enabled. Conditionally build it so that we do not get a clippy warning when building without these features.

Remove the TODO; refactor matching to correctly handle conditionally built `Sled` variants. Use `unreachable` instead of `unimplemented` with a comment hinting that this is a bug, this makes it explicit, both at runtime and when reading the code, that this match arm should not be hit.

Clippy emits error: comparing trait object pointers compares a non-unique vtable address The vtable is an implementation detail, it may change in future. we should not be comparing vtable addresses for equality. Instead we can get a pointer to the data field of a fat pointer and compare on that.

Clippy emits: warning: Question mark operator is useless here No need to use the `?` operator inside an `Ok()` statement when returning, just return directly.

As suggested by Clippy, use the `vec!` macro directly instead of declaring a mutable vector and pushing elements onto it.

As suggested by Clippy us `map` instead of `and_then` with an inner option.

`lazy_static` is not imported, this error is hidden behind the `compact_filters` feature flag.

Found by Clippy, we don't need this `macro_use` statement.

We build against various targets on CI, in order to not abuse CI its nice to see if code is good before pushing. Add a script that runs `cargo check` with various combinations of features and targets in order to enable thorough checking of the project source code during development.

@afilini

Recently we shortened the first line of a multi-line string and failed to re-align the rest of the lines. Thanks @afilini

@afilini

If we give the `DummySigner` a valid identifier then we can use this to do comparison. Half the time we do comparison we only have a `dyn Signer` so we cannot use `PartialEq`, add a helper function to check equality (this is in test code so its not toooo ugly). Thanks @afilini for the suggestion.

Currently we have a unit test to test that signers are sorted by ordering. We call `add_external` to add them but currently we add them in the same order we expect them to be in. This means if the implementation happens to insert them simply in the order they are added (i.e. insert to end of list) then this test will still pass. Insert in a mixed order, including one lower followed by one higher - this ensures we are not inserting at the front or at the back but are actually sorting based on the `SignerOrdering`.

tcharding · 2021-02-24T02:44:21Z

Please note, during the last round of changes I noticed a potential problem with one of the unit tests, turns out not to be a problem but the fix is trivial and makes the unit test more robust. Its such a trivial change I pushed it on top of this PR but technically the PR is now not just clippy fixes. If required I can do a separate PR.

Please review the final patch bda416d Use mixed order insertions - unit test fix not clippy fix.

notmandatory

ACK bda416d

tcharding mentioned this pull request Feb 15, 2021

Fix clippy warnings for new stable rust 1.50.0 #282

Merged

9 tasks

tcharding force-pushed the 126-clippy branch from cba76cc to 1712da9 Compare February 15, 2021 08:19

notmandatory reviewed Feb 15, 2021

View reviewed changes

tcharding force-pushed the 126-clippy branch 2 times, most recently from b58fcf5 to a813928 Compare February 17, 2021 05:32

afilini reviewed Feb 22, 2021

View reviewed changes

tcharding added 19 commits February 24, 2021 13:30

Use unwrap_or_else

5eaa3b0

As directed by clippy use `unwrap_or_else` in order to take advantage of lazy evaluation.

Use ! is_empty instead of len > 0

2057c35

As directed by clippy use `!a.is_empty()` instead of `a.len() > 0`.

Remove static lifetime

3a0a1e6

const str types do not need an explicit lifetime, remove it. Found by clippy.

Remove redundant calls to clone

0e99d02

No need to clone copy types, found by clippy.

Remove needles explicit reference

2afc9fa

Clippy emits warning: warning: needlessly taken reference of both operands Remove the explicit reference's as suggested.

Use count instead of collect and len

79cab93

Clippy emits warning: warning: avoid using `collect()` when not needed As suggested by clippy just use `count` directly on the iterator instead of `collect` followed by `len`.

Use next instead of nth(0)

824b00c

As suggested by clippy we can use `.next()` on an iterator instead of `nth(0)`. Although arguably no clearer, and certainly no worse, it keeps clippy quiet and a clean lint is a good thing.

Use default pattern

35184e6

Clippy emits warning: warning: field assignment outside of initializer for an instance created with Default::default() Do as suggested by clippy and use the default init pattern. ``` let foo = Foo { bar: ..., Default::default() } ```

Conditionally compile constructor

343e97d

The `ChunksIterator` constructor is only used when either `electrum` or `esplora` features are enabled. Conditionally build it so that we do not get a clippy warning when building without these features.

Remove useless question mark operator

24df438

Clippy emits: warning: Question mark operator is useless here No need to use the `?` operator inside an `Ok()` statement when returning, just return directly.

Use vec! instead of mut and push

e35601b

As suggested by Clippy, use the `vec!` macro directly instead of declaring a mutable vector and pushing elements onto it.

Use map instead of and_then

bfe29c4

As suggested by Clippy us `map` instead of `and_then` with an inner option.

Use lazy_static

2b5e177

`lazy_static` is not imported, this error is hidden behind the `compact_filters` feature flag.

Remove unused macro_use

97ad0f1

Found by Clippy, we don't need this `macro_use` statement.

Align multi-line string

d2a094a

Recently we shortened the first line of a multi-line string and failed to re-align the rest of the lines. Thanks @afilini

tcharding force-pushed the 126-clippy branch 2 times, most recently from c54418c to bf9660a Compare February 24, 2021 02:36

tcharding force-pushed the 126-clippy branch from bf9660a to a838c2b Compare February 24, 2021 02:38

notmandatory approved these changes Feb 24, 2021

View reviewed changes

afilini merged commit 1cbd47b into bitcoindevkit:master Feb 26, 2021

tcharding deleted the 126-clippy branch August 17, 2021 06:08

Conversation

tcharding commented Feb 13, 2021

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afilini commented Feb 22, 2021

Uh oh!

tcharding commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Feb 24, 2021

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Feb 24, 2021 •

edited

Loading