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.

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

verbose_bit_mask is questionable #5633

Closed
mahkoh opened this issue May 22, 2020 · 4 comments · Fixed by #6036
Closed

verbose_bit_mask is questionable #5633

mahkoh opened this issue May 22, 2020 · 4 comments · Fixed by #6036
Labels
good-first-issue These issues are a good way to get started with Clippy

Comments

@mahkoh
Copy link

mahkoh commented May 22, 2020

Consider the following:

pub const fn WTERMSIG(s: c::c_int) -> c::c_int {
    s & 0x7f
}

pub const fn WIFEXITED(s: c::c_int) -> bool {
    s & 0x7f == 0 //   <--- try: `s.trailing_zeros() >= 7`
}

The documentation for verbose_bit_mask has the following example:

x.trailing_zeros() > 4 is much clearer than x & 15 == 0

But this statement is wrong in the general case. Code such as x & 15 is often accompanied by other bit-fiddling to extract certain parts of the integer such as in the example above. Using trailing_zeros in one place and & 15 in the other makes the code as a whole much less clear.

verbose_bit_mask should be disabled by default.

cc @oli-obk

@flip1995 flip1995 added the good-first-issue These issues are a good way to get started with Clippy label May 25, 2020
@oli-obk
Copy link
Contributor

oli-obk commented May 26, 2020

While I can imagine that there are situations where the lint would make the code less readable, I don't think the above example is a motivating example for disabling the lint. While the fix for the situation is not the one suggested by the lint, using

pub const fn WIFEXITED(s: c::c_int) -> bool {
    WTERMSIG(s) == 0
}

has less duplication and is much clearer imo. The lint succeeded in highlighting this situation that deserved improvement, so to me it has done its job.

@oli-obk oli-obk added S-needs-discussion Status: Needs further discussion before merging or work can be started and removed good-first-issue These issues are a good way to get started with Clippy labels May 26, 2020
@mahkoh
Copy link
Author

mahkoh commented May 26, 2020

The lint succeeded in highlighting this situation that deserved improvement, so to me it has done its job.

fn is_type_a(x: u32) -> bool {
    x & 0xff00 == 0
}

fn is_type_b(x: u32) -> bool {
    x & 0x00ff == 0   // try: `x.trailing_zeros() >= 8`
}

Which situation is clippy highlighting in this example?

@mahkoh
Copy link
Author

mahkoh commented May 26, 2020

@oli-obk The reason I tagged you was so that you could explain why you added this lint in the first place. The commit message contains no information. I'm not aware of ever seeing any piece of code where someone used masking where using a dedicated function for counting trailing zeros would have been clearer.

@oli-obk
Copy link
Contributor

oli-obk commented May 26, 2020

This new example shows where the current lint fails indeed. While I personally would like it extended to lint x & 0xff00 == 0 and suggest x.leading_zeros() >= 8, too, I agree that this is a very opinionated lint and be moved to pedantic

@oli-obk oli-obk added good-first-issue These issues are a good way to get started with Clippy and removed S-needs-discussion Status: Needs further discussion before merging or work can be started labels May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants