refactor(storage-proofs): fix clippy warnings #1147

tcharding · 2020-06-03T10:17:50Z

Fix all clippy warnings produced with the following invocation:

cargo clippy --all-targets --all-features -- -D warnings

None of the fixes are particularly noteworthy except for:

Usage of combinators instead of array index, arguably makes the test code harder to read: 3592d8c
Add test to criterion_group (see discussion below): 889f4ad
Allow unit_arg lint for usage of black_box: 5d67be3

Closes: #1123

vmx

Just a few minor comments.

vmx · 2020-06-03T14:01:49Z

storage-proofs/core/benches/drgraph.rs

@@ -15,7 +15,8 @@ fn drgraph(c: &mut Criterion) {

                b.iter(|| {
                    let mut parents = vec![0; 6];
-                    black_box(graph.parents(2, &mut parents).unwrap());
+                    // parents() never fails, call is_ok() to quieten clippy.


It's out of scope for this PR, but wouldn't it make sense create a new PR which changes parents() to not return a Result, but just nothing?

I initially thought that (even had that written in the commit log at one stage), also during the remove unwrap work this came up. parents is a trait method, I didn't dig into it but assumed the result was used by other implementers of the the trait, otherwise why would it be there in the first place? I can definitely look into this further now you mentioned it though.

vmx · 2020-06-03T14:03:33Z

storage-proofs/core/benches/pedersen.rs

@@ -157,60 +155,6 @@ fn pedersen_circuit_benchmark(c: &mut Criterion) {
    );
 }

-fn pedersen_md_circuit_benchmark(c: &mut Criterion) {


Are you sure you want to remove this one? I'd rather add this to the criterion_group!() call below. You could check with the original author, if it was a mistake not adding it, or if it should be removed.

Good point, @laser git blame highlights you as the developer who added the macro call criterion_group. Do you remember if there was a reason why pedersen_md_circuit_benchmark() was not added to it please?

vmx · 2020-06-03T14:07:48Z

storage-proofs/core/src/crypto/pedersen.rs

@@ -405,8 +405,8 @@ mod tests {
            let hashed = pedersen_md_no_padding(&flat);

            let mut hasher = Hasher::new(&x[0]).unwrap();
-            for k in 1..5 {
-                hasher.update(&x[k]).unwrap();
+            for val in x.iter().take(5).skip(1) {


Just a personal nitpick (no need to change it). I would write it as x.iter().skip(1).take(5). I needed a while to parse this one, I read the current version as "take the first five elements, ohhh wait, skip the first one".

Agreed. Highlighting that you are correct is the fact that the 5 is misleading because you translated it to x.iter().skip(1).take(5) which iis different, we actually only want 4 items. I'll use x.iter().skip(1).take(4) for this and I'll use this order for this sort of combinator in future, thanks for the lesson.

tcharding · 2020-06-03T23:32:41Z

Force pushed the combinator change, will wait on further comment before fixing the unused function change. Please do not merge (do you guys have a process for this, would you prefer me to change this to a draft PR to sort it out or is the discussion thread enough?) Thanks.

tcharding · 2020-06-03T23:34:07Z

While we are on the topic of process, do you guys have a preference for force pushing / not force pushing? Do you follow the 'don't change history' philosophy or the 'maintain clean discrete patch sets' philosophy?

tcharding · 2020-06-03T23:35:54Z

Oh, and thanks for the review @vmx, appreciate it.

laser · 2020-06-04T05:12:04Z

Hi Tobin, Nope - I have no idea why it wasn’t added. Probably an accidental omission / oversight. Erin

…

On Wed, Jun 3, 2020, at 4:23 PM, Tobin C. Harding wrote: ***@***.**** commented on this pull request. In storage-proofs/core/benches/pedersen.rs <#1147 (comment)>: > @@ -157,60 +155,6 @@ fn pedersen_circuit_benchmark(c: &mut Criterion) { ); } -fn pedersen_md_circuit_benchmark(c: &mut Criterion) { Good point, @laser <https://github.com/laser> `git blame` highlights you as the developer who added the macro call `criterion_group`. Do you remember if there was a reason why `pedersen_md_circuit_benchmark()` was not added to it please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1147 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGX6G2LKG7GK7ODX5GSZHDRU3LNRANCNFSM4NRQ3HAA>.

vmx · 2020-06-04T11:55:12Z

Do you follow the 'don't change history' philosophy or the 'maintain clean discrete patch sets' philosophy?

I can't speak for the whole team. But I personally prefer clean discrete patch sets. Though it also has it's limits. If you e.g. do a rebase and a change and force push, it might make it hard for reviewers what actually changes. What I do in such cases is that I just push a commit with review comments addressed and once everything is reviewed a forced push with a clean history.

Havind said that, we also happily squash commits, so feel free to just keep adding things and we'll do a squash commit at the end.

tcharding · 2020-06-04T22:21:54Z

Hi Tobin, Nope - I have no idea why it wasn’t added. Probably an accidental omission / oversight. Erin

Cool, thanks.

tcharding · 2020-06-04T22:22:13Z

Do you follow the 'don't change history' philosophy or the 'maintain clean discrete patch sets' philosophy?

I can't speak for the whole team. But I personally prefer clean discrete patch sets. Though it also has it's limits. If you e.g. do a rebase and a change and force push, it might make it hard for reviewers what actually changes. What I do in such cases is that I just push a commit with review comments addressed and once everything is reviewed a forced push with a clean history.

Havind said that, we also happily squash commits, so feel free to just keep adding things and we'll do a squash commit at the end.

Awesome, thanks.

tcharding · 2020-06-04T22:31:53Z

I have updated the patch order, and amended the criterion_group patch based on responses above. Force pushed the whole thing and updated the initial PR comment to reflect this.

Thanks.

vmx

Thanks a lot @tcharding for those changes, they look good! Please bear with us, it might take a bit until this PR is merged. We are currently in a pretty busy/critical phase. But I'll make sure that it's get merged eventually.

tcharding · 2020-06-14T23:47:45Z

Thanks for the explanation @vmx, no worries at all. No rush, when you are ready just holla at me and I can rebase. I'm going to keep chipping away at little issues as I can, if there is anyway I can make your lives (as reviewers) easier just let me know.

cryptonemo

@tcharding This is looking good, sorry for the delay on review. I'm going to rebase it against current master and merge it at some point today, unless you rebase it first. Thanks!

clippy emits error: error: this `else { if .. }` block can be collapsed Do what clippy suggests, collapse the else if block.

In test module we bring into scope `rand` but do not use it (thread_rng() is called using a qualified path). Remove the unused `rand` import.

As suggested by clippy we can use `iter()` instead of `into_iter() in a bunch of places.`

Clippy emits a bunch of warnings of type: warning: useless use of `vec!` And suggests that we can use a slice direcly help: you can use a slice directly: `&[None; 256]` Do as clippy suggests; use slices directly.

Found with clippy; we are using a for loop but the current lint level requires us to use an iterator. As suggested; use an iterator instead of the for loop.

Found with clippy; `bytes_to_fr` does not need a mutable reference.

Remove unnecessary usage of `into()`, found by clippy.

Remove unnecessary usage of `clone()`, found by clippy.

Clippy emits error: error: you don't need to add `&` to all patterns Along with the suggestion to dereference the expression instead of prefixing all patters with `&`. Do as suggested by clippy and dereference the expressions.

Clippy emits two errors of type: error: You are using an explicit closure for copying elements Along with the suggestion to us `copied` instead. Do as clippy suggests.

The function `pedersen_md_circuit_benchmark` is defined but not used, it appears that this function should be added to the `criterion_group` marcro call. Found by clippy.

`tempfile` is brought into scope but is never used. Found by clippy.

Plain old `return;` is idiomatic Rust these days, found by clippy.

We declare a local buffer and never use it. Found by clippy.

Clippy emits error: error: long literal lacking separators Use separators, as suggested by clippy, to assist reading.

Clippy emits warning: warning: passing a unit value to a function The argument in question is not actually used by the function `circuit` (if I've jumped to the correct definition). We can safely pass in unit `()` instead. Does this result in any loss of meaning?

Clippy emits warning warning: digits grouped inconsistently by underscores Add an underscore, as suggested, in order to be consistent.

Import is unused; found by clippy.

Clippy emits error: error: identical conversion Remove, as suggested by clippy, unnecessary `into_iter()`.

Clippy emits error: error: the loop variable `i` is used to index `val` Use an iterator, as suggested, to loop over the arrays of bytes. Use combinators `skip` and `take` as needed.

Clippy emits error: error: casting integer literal to `f64` is unnecessary Use `32_f64` as suggested instead of casting to an `f64`.

Clippy emits error: error: the operation is ineffective. Consider reducing it to `127` This is caused because we use `1 * 127` in order to be consistent with other usages i.e., `2 * 127` and `8 * 127`. Clippy is wrong in this instance, the ineffective operation makes the code easier to read. Instruct clippy to allow this lint.

Clippy emits warning/error; useless use of `vec!` Do as suggested by clippy and use a slice directly.

Clippy emits warning: warning: redundant clone Do as clippy suggests; remove redundant calls to `clone()`

Clippy emits error: error: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type This error is caused by code that is mildly convoluted by the fact that the only method on `TempDir` that returns a `PathBuf` consumes self. We cannot use that because at the call site we cannot consume the temp dir. To get around this we do a little dance of getting a reference to a `Path` and then calling `to_path_buf()` on it. We end up with code that looks kind of funky: self.cache_dire.path().to_path_buf() That's the best I could come up with :)

`unwrap_or_else()` is more performant than `unwrap_or`. Found by clippy.

Clippy emits warning: warning: this `.into_iter()` call is equivalent to `.iter()` and will not move the `slice` Do as clippy suggests and use `iter()` directly.

Clippy emits error: error: written amount is not handled. Use `Write::write_all` instead Use `write_all` as clippy suggests since we do not use the return result.

Clippy emits warning: warning: identical conversion Remove the call to `into_iter()`, as suggested by clippy, and use `iter()`.

Clippy emits error: error: use of `expect` followed by a function call Do as suggested by clippy and use `unwrap_or_else()` coupled with `panic!` instead of calling `format!` as an argument to `expect()`.

Clippy emits warning: warning: long literal lacking separators Add separators as suggested by clippy to assist reading.

We use a couple of strict float comparisons to test. In each case the strict comparison is ok. Add a lint ignore and also comment each site that does strict float comparison.

We use the `black_box` function in a bunch of places, at times the function argument returns unit, this is to be expected since the argument is not used for its return value. Instruct clippy to set the `unit_arg` lint to allow.

tcharding · 2020-06-18T21:35:29Z

lol you beat me to it, was just working on it now :)

tcharding requested review from cryptonemo, dignifiedquire and porcuquine as code owners June 3, 2020 10:17

vmx reviewed Jun 3, 2020

View reviewed changes

tcharding changed the title ~~Fix clippy warnings (storage-proofs/core)~~ refactor(storage-proofs): fix clippy warnings Jun 10, 2020

tcharding requested a review from vmx June 11, 2020 23:21

vmx previously approved these changes Jun 12, 2020

View reviewed changes

tcharding dismissed vmx’s stale review via 91e695d June 16, 2020 06:59

cryptonemo previously approved these changes Jun 18, 2020

View reviewed changes

cryptonemo dismissed their stale review via 3937540 June 18, 2020 13:25

tcharding added 11 commits June 18, 2020 13:26

style(storage-proofs): collapse else if block

6861b07

clippy emits error: error: this `else { if .. }` block can be collapsed Do what clippy suggests, collapse the else if block.

style(storage-proofs): remove unused import

68ec2f7

In test module we bring into scope `rand` but do not use it (thread_rng() is called using a qualified path). Remove the unused `rand` import.

refactor(storage-proofs): use iter() instead of into_iter()

339ed4e

As suggested by clippy we can use `iter()` instead of `into_iter() in a bunch of places.`

refactor(storage-proofs): use slice directly

6b00682

Clippy emits a bunch of warnings of type: warning: useless use of `vec!` And suggests that we can use a slice direcly help: you can use a slice directly: `&[None; 256]` Do as clippy suggests; use slices directly.

refactor(storage-proofs): use iterator instead of for loop

8fc2a3e

Found with clippy; we are using a for loop but the current lint level requires us to use an iterator. As suggested; use an iterator instead of the for loop.

refactor(storage-proofs): remove unneeded mutable reference

a5ff82f

Found with clippy; `bytes_to_fr` does not need a mutable reference.

refactor(storage-proofs): remove unnecessary into()

71f094d

Remove unnecessary usage of `into()`, found by clippy.

refactor(storage-proofs): remove unnecessary clone()

5f5c4cd

Remove unnecessary usage of `clone()`, found by clippy.

refactor(storage-proofs): use dedicated copied method

26f6675

Clippy emits two errors of type: error: You are using an explicit closure for copying elements Along with the suggestion to us `copied` instead. Do as clippy suggests.

refactor(storage-proofs): include function in criterion_group

8d1b060

The function `pedersen_md_circuit_benchmark` is defined but not used, it appears that this function should be added to the `criterion_group` marcro call. Found by clippy.

tcharding added 22 commits June 18, 2020 13:26

refactor(storage-proofs): remove unnecessary use

1ca55ff

`tempfile` is brought into scope but is never used. Found by clippy.

refactor(storage-proofs): remove unnecessary ()

27a4174

Plain old `return;` is idiomatic Rust these days, found by clippy.

refactor(storage-proofs): remove unused local variable

afadb20

We declare a local buffer and never use it. Found by clippy.

refactor(storage-proofs): use separators

097349c

Clippy emits error: error: long literal lacking separators Use separators, as suggested by clippy, to assist reading.

refactor(filecoin-proofs): use consistent underscoring

ff23373

Clippy emits warning warning: digits grouped inconsistently by underscores Add an underscore, as suggested, in order to be consistent.

refactor(filecoin-proofs): remove redundant import

c43cf3c

Import is unused; found by clippy.

refactor(filecoin-proofs): remove unnecessary into_iter()

5f80973

Clippy emits error: error: identical conversion Remove, as suggested by clippy, unnecessary `into_iter()`.

refactor(filecoin-proofs): use iterator instead of indexing

ea04524

Clippy emits error: error: the loop variable `i` is used to index `val` Use an iterator, as suggested, to loop over the arrays of bytes. Use combinators `skip` and `take` as needed.

refactor(filecoin-proofs): use _f64 type

6c2b6b3

Clippy emits error: error: casting integer literal to `f64` is unnecessary Use `32_f64` as suggested instead of casting to an `f64`.

refactor(filecoin-proofs): remove useless use of vec!

108f8df

Clippy emits warning/error; useless use of `vec!` Do as suggested by clippy and use a slice directly.

refactor(filecoin-proofs): remove redundant clone

255d681

Clippy emits warning: warning: redundant clone Do as clippy suggests; remove redundant calls to `clone()`

refactor(filecoin-proofs): use unwrap_or_else

d30ab86

`unwrap_or_else()` is more performant than `unwrap_or`. Found by clippy.

refactor(filecoin-proofs): use iter() directly

f9d5822

Clippy emits warning: warning: this `.into_iter()` call is equivalent to `.iter()` and will not move the `slice` Do as clippy suggests and use `iter()` directly.

refactor(filecoin-proofs): use write_all

3c2a891

Clippy emits error: error: written amount is not handled. Use `Write::write_all` instead Use `write_all` as clippy suggests since we do not use the return result.

refactor(filecoin-proofs): remove identical conversion

61eed54

Clippy emits warning: warning: identical conversion Remove the call to `into_iter()`, as suggested by clippy, and use `iter()`.

refactor(filecoin-proofs): no function call after expect

6e198bb

Clippy emits error: error: use of `expect` followed by a function call Do as suggested by clippy and use `unwrap_or_else()` coupled with `panic!` instead of calling `format!` as an argument to `expect()`.

refactor(fil-proofs-tooling): add separators

7aba1d0

Clippy emits warning: warning: long literal lacking separators Add separators as suggested by clippy to assist reading.

refactor(fil-proofs-tooling): all clippy lint float_cmp

ed71a56

We use a couple of strict float comparisons to test. In each case the strict comparison is ok. Add a lint ignore and also comment each site that does strict float comparison.

refactor(storage-proofs): all clippy lint unit_arg

6aa355f

We use the `black_box` function in a bunch of places, at times the function argument returns unit, this is to be expected since the argument is not used for its return value. Instruct clippy to set the `unit_arg` lint to allow.

porcuquine approved these changes Jun 18, 2020

View reviewed changes

cryptonemo merged commit a446061 into filecoin-project:master Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(storage-proofs): fix clippy warnings #1147

refactor(storage-proofs): fix clippy warnings #1147

tcharding commented Jun 3, 2020 •

edited

Loading

vmx left a comment

vmx Jun 3, 2020

tcharding Jun 3, 2020

vmx Jun 3, 2020

tcharding Jun 3, 2020

vmx Jun 3, 2020

tcharding Jun 3, 2020

tcharding commented Jun 3, 2020

tcharding commented Jun 3, 2020

tcharding commented Jun 3, 2020

laser commented Jun 4, 2020 via email

vmx commented Jun 4, 2020

tcharding commented Jun 4, 2020

tcharding commented Jun 4, 2020

tcharding commented Jun 4, 2020

vmx left a comment

tcharding commented Jun 14, 2020

cryptonemo left a comment

tcharding commented Jun 18, 2020

refactor(storage-proofs): fix clippy warnings #1147

refactor(storage-proofs): fix clippy warnings #1147

Conversation

tcharding commented Jun 3, 2020 • edited Loading

vmx left a comment

Choose a reason for hiding this comment

vmx Jun 3, 2020

Choose a reason for hiding this comment

tcharding Jun 3, 2020

Choose a reason for hiding this comment

vmx Jun 3, 2020

Choose a reason for hiding this comment

tcharding Jun 3, 2020

Choose a reason for hiding this comment

vmx Jun 3, 2020

Choose a reason for hiding this comment

tcharding Jun 3, 2020

Choose a reason for hiding this comment

tcharding commented Jun 3, 2020

tcharding commented Jun 3, 2020

tcharding commented Jun 3, 2020

laser commented Jun 4, 2020 via email

vmx commented Jun 4, 2020

tcharding commented Jun 4, 2020

tcharding commented Jun 4, 2020

tcharding commented Jun 4, 2020

vmx left a comment

Choose a reason for hiding this comment

tcharding commented Jun 14, 2020

cryptonemo left a comment

Choose a reason for hiding this comment

tcharding commented Jun 18, 2020

tcharding commented Jun 3, 2020 •

edited

Loading