-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add [manual_rem_euclid
] lint
#9031
Conversation
r? @Jarcho (rust-highfive has picked a reviewer for you, use r? to override) |
return None; | ||
}; | ||
|
||
if int_const > FullInt::S(0) { |
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 would rewrite this to not have the unwrap()
. Something like
let val = match int_const {
FullInt::S(s) => s.try_into(),
FullInt::U(u) => Some(u)
}
Some((val?, other_op))
There might be a better way of doing it.
--
Edit: Just a comment. Not a review.
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.
Agreed, believe this is in the spirit of that 75ed0c9
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.
You can match on the constant and skip the bounds check here. i.e.
match x {
S(x) => x.try_into().ok(),
U(x) => Some(x),
}
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, realized you can't do x % 0
because zero divisor is invalid, so the equal checks for the consts makes sure the const is nonzero so some checks were completely unnecessary, thanks
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.
rem_euclid
is only const as of 1.52
. You'll need to add a call to clippy_utils::in_constant
with a second msrv check.
There should be a call to in_external_macro, and an msrv test for the const version with the msrv set between the two versions. Otherwise it all looks good. |
Looks good now. Thank you. @bors r+ |
📌 Commit df26c3f has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Closes #8883
Adds a lint for checking manual use of
rem_euclid(n)
changelog: Add [
manual_rem_euclid
] lint