Dev bp clippy #18
Draft
Dev bp clippy #18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cydhra
reviewed
Jan 8, 2025
| @@ -213,14 +213,14 @@ fn test_custom_iter_behavior() { | |||
| assert!(iter.advance_by(6).is_err()); | |||
| assert!(iter.advance_back_by(5).is_ok()); | |||
|
|
|||
| assert_eq!(bv.iter().skip(2).next(), Some(0)); | |||
There was a problem hiding this comment.
Please revert this and suppress the clippy correction. This intentionally calls skip() and next() to verify skip behaves correctly. This test should exist regardless of whether nth() internally uses skip (which I think it doesn't). Same for the change a few lines down.
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_custom_iter_behavior() { | ||
| let ef = EliasFanoVec::from_slice(&vec![0, 1, 2, 3, 4, 5, 6, 7, 8]); | ||
| assert_eq!(ef.iter().skip(2).next(), Some(2)); |
There was a problem hiding this comment.
Same here: skip().next() is called intentionally to test the skip function over nth(). Same a few lines below.
| @@ -94,8 +94,8 @@ impl WaveletMatrix { | |||
|
|
|||
| for (level, data) in data.iter_mut().enumerate() { | |||
| let mut total_zeros = 0; | |||
| for i in 0..num_elements { | |||
| if bit_lookup(permutation[i], element_len - level - 1) == 0 { | |||
| for (i, p) in permutation.iter().enumerate().take(num_elements) { | |||
There was a problem hiding this comment.
is take() required here? is i required here?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here's a draft PR applying suggested clippy changes to the dev_bp branch to make
cargo clippyproduce no output and besides that I tried to follow clippy suggestions in tests and benches.Many of the clippy changes are in tests. A lot of them have to do with changing manual indexing to using iterators, or not using
vec!for sample data.In main code:
I noticed that unfortunately there are some spurious diffs due to whitespace changes - I only use the standard Rust formatter though. I think that's mostly restricted to the new trees module. Most of the clippy changes in the trees section involve changing
and_thenintomapand a few redundant closures.div_ceilis an interesting one; there were various cases where it was implemented manually. There are a few changes of manual indexing to iter() in the wavelet matrix code.I am curious whether there is any performance impact; I haven't done a benchmark compare yet.
Finally one change that seems silly: the additional brackets are what clippy suggests though they seem entirely redundant here.