-
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
Resolve false positives of unnecessary_cast for non-decimal integers #5257
Conversation
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.
Please also add test cases for 0b
and 0o
clippy_lints/src/types.rs
Outdated
return; | ||
if let Some(num_lit) = NumericLiteral::from_lit_kind(&src, &lit.node) { | ||
if from_nbits != 0 && to_nbits != 0 && from_nbits <= to_nbits && | ||
num_lit.radix != Radix::Hexadecimal { |
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 we also want to not lint this for 0o
and 0b
, so you could add a is_decimal()
method to the NumericLiteral
struct.
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 0o
and 0b
, the suffixes _f32
or _f64
would not be interpreted as part of the literal value such that the cast is indeed unnecessary. Am I correct?
However, it would probably make sense to print the suggestion also in octal or binary format. I will work on that and modify the PR accordingly.
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.
Same behavior for 0b
and 0o
Playground:
warning: casting integer literal to `f32` is unnecessary
--> src/main.rs:2:13
|
2 | let _ = 0b10101 as f32;
| ^^^^^^^^^^^^^^ help: try: `21_f32`
|
= note: `#[warn(clippy::unnecessary_cast)]` on by default
If any other radix, than decimal is used, it is most definitely intended, so suggesting to change it to decimal+_f32
is not something we want to do. Since binary and octal float literals are not supported, we also want to bail out there.
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.
Ok, thanks for the additional pointers. I will include the binary and octal case.
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.
Done.
@flip1995 I moved the NumericLiteral to its own module and now also handle the binary and octal cases. I needed to make some fields public such that they remain accessible. |
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.
LGTM overall. Only 1 NIT left.
Thanks a lot for the feedback, that was very helpful. |
☔ The latest upstream changes (presumably #5246) made this pull request unmergeable. Please resolve the merge conflicts. |
Thanks, a rebase is needed, because of #5246. After that this is good to go. |
@flip1995 Rebase done. |
Thanks! @bors r+ |
📌 Commit 185fa0d has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This PR resolves false positives of
unnecessary_cast
for hexadecimal integers to floats and adds a corresponding test case.Fixes: #5220
changelog: none