Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extend precedence to lint for masking bits and shifts #8735

Closed
wants to merge 1 commit into from
Closed

Extend precedence to lint for masking bits and shifts #8735

wants to merge 1 commit into from

Conversation

goth-turtle
Copy link
Contributor

Currently this lints for any expression _ & _ << _ or _ & _ >> _, and suggests adding parentheses, like so: (_ & _) << _.
Also, im not really sure about my wording with the documentation i added, so id appreciate some feedback on that. Thanks!

changelog: extended [precedence] to lint for bitmasking with shifting without parentheses

fixes #6632

@goth-turtle
Extend precedence to lint for masking bits and shifts
968bde1 
This changes precedence to lint for expressions like `x & 0xF000 >> 12`,
where the user likely meant to mask before shifting, and suggests adding
parentheses around the binary and.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 22, 2022
@Manishearth
Copy link
Member

r? @Jarcho

(I'm travelling rn and might not be able to do full reviews, can rubber-stamp after you've had a look at it)

@Jarcho
Copy link
Contributor

Jarcho commented Apr 26, 2022

You'll need to switch the suggestion to x & (y >> z)? The precedence lint is meant as a note that the code as written may be easy to misread, not that the code is necessarily wrong.

I'll give a proper review tomorrow.

Jarcho
Jarcho requested changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@Jarcho Jarcho left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished an initial read of the changes. The current implementation will lint twice on expression such as a + b & x >> d.

I'd also like to see this extended to handle any combination of &, |, and ^ with either shift operation. The approach would be to change the match expression to match (op, get_bin_opt(left), get_bin_opt(right)) and then fill in arms like (BitAnd | BitOr | BitXor, Some(Shl | Shr | Add | ...), Some(Shl | Shr | Add | ...)). get_bin_opt will need to be written as well.

clippy_lints/src/precedence.rs
return;
}
let mut applicability = Applicability::MachineApplicable;
match (is_arith_expr(left), is_arith_expr(right)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the match expression that would need to change.

@giraffate
Copy link
Contributor

@goth-turtle ping from triage. Can you have any question on this?

@bors
Copy link
Contributor

bors commented Aug 29, 2022

☔ The latest upstream changes (presumably #9388) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas
Copy link
Member

blyxyas commented Oct 6, 2023

@goth-turtle closing this it has been inactive for 403 days. Closing this as (from your contribution history) it seems like you're no longer interested in OSS (nothing wrong about that ❤️).

If you return to the community, and are interested in continuing this PR, don't hesitate to reopen this PR (●ↀωↀ●)

@blyxyas blyxyas closed this Oct 6, 2023
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 9, 2023
@Natsume-Neko Natsume-Neko mentioned this pull request Nov 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jarcho Jarcho Jarcho requested changes

Assignees

@Manishearth Manishearth

Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint masking bits and shift
8 participants
@goth-turtle @rust-highfive @Manishearth @Jarcho @giraffate @bors @blyxyas @flip1995