-
Notifications
You must be signed in to change notification settings - Fork 203
silenced a bunch of misleading Clippy warnings #176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3e25c6e
to
5326052
Compare
@cuviper These diffs are a bit gratuitous but I think they'll be a net positive. I chose to silence Clippy rather than change working code any more than necessary. Hopefully cutting down the Clippy noise will help the next person. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for making clippy more useful here.
src/algorithms.rs
Outdated
@@ -347,6 +347,7 @@ fn bigint_from_slice(slice: &[BigDigit]) -> BigInt { | |||
BigInt::from(biguint_from_vec(slice.to_vec())) | |||
} | |||
|
|||
#[allow(clippy::many_single_char_names)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style-wise, I'd prefer attributes to be placed after doc comments, directly before the code they modify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got all these, I think.
value.to_biguint().ok_or(TryFromBigIntError::new(())) | ||
value | ||
.to_biguint() | ||
.ok_or_else(|| TryFromBigIntError::new(())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think clippy is being overly fussy here since TryFromBigIntError<()>
is a ZST, and new
isn't doing anything significant, but I guess there's no harm in wrapping this. Another option would be to write this as a direct struct initialization for the ZST, but I think it won't matter for performance either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Yeah, I think it's probably the cleanest way to shut Clippy up.
1 => u64::from(v.data[0]), | ||
1 => | ||
{ | ||
#[allow(clippy::useless_conversion)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the attribute repeated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mistake. Fixed.
5326052
to
a2b160c
Compare
bors r+ |
There were a bunch of code quality Clippy warnings that didn't apply for various reasons.
There was an ok_or() that could have been an ok_or_else() that Clippy caught. Turns out it's for a ZST, so doesn't matter, but was "fixed" anyway to silence Clippy.
This will hopefully make "cargo clippy" more useful for the next person.