Rust-clippy: verbose_bit_mask is questionable

Created on 22 May 2020  路  4Comments  路  Source: rust-lang/rust-clippy

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

good-first-issue

Most helpful comment

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

All 4 comments

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.

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?

@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.

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

Was this page helpful?
0 / 5 - 0 ratings