rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

Add lints against more manual integer ops where direct methods exist

Open ivan-shrimp opened this issue 1 year ago • 1 comments

What it does

Checks for more manual reimplementations of various integer operations that are available as methods. (This can, and probably should, be separated into different lints.)

An existing lint of this type is manual_rem_euclid, preferring rem_euclid over ((a % b) + b) % b.

  • [ ] div_ceil: (a + (b - 1)) / b https://github.com/rust-lang/rust-clippy/pull/12987
  • [ ] unsigned checked_div: b != 0 check before a / b
  • [ ] unsigned checked_sub: a >= b check before a - b
  • [ ] checked_ilog{|2|10}: a > 0 check before ilog{|2|10}
  • [ ] is_power_of_two: count_ones() == 1
  • [ ] is_power_of_two: a & (a - 1) == 0
  • [ ] ilog2: CONST - a.leading_zeros()
  • [ ] ilog2 and ilog10: ilog(2) and ilog(10)
  • [ ] {count|leading|trailing}_{zeros|ones}: any counterpart on !a or a.reverse_bits()
  • [ ] rotate_{left|right}: (a << n) | (a >> (BITS - n)) https://github.com/rust-lang/rust-clippy/pull/12983

(We can also add midpoint as replacement for the overflow-incorrect (a + b) / 2, once that's stabilized.)

Advantage

  • Implementations in the standard library are often better optimized, such as using intrinsics or unchecked operations internally.
  • Edge cases like overflows may not be handled by the manual implementations properly, leading to panics in debug builds.
  • This can simplify code that may otherwise look hacky.

Drawbacks

  • Some code may be directly translated from, e.g. old C code. Changing them to method calls may harm readability.
  • The replacements are sometimes longer than the original.
  • Some edge case properties might be intended, not accidental.
  • There simply are too many poor man's implementations of intrinsics. We will need to prioritize common cases.

Example

let [a, b, c]: [u32; 3] = bar();
let div = (a + (b - 1)) / b;
if a > 0 { foo(a - 1) }
if b >= c { foo(b - c) }
if a != 0 { foo(b / a) }
if a != 0 { foo(a.ilog2()) }

let single_bit_v1: bool = a.count_ones() == 1;
let single_bit_v2: bool = a & (a - 1) == 0;
let log = 31 - a.leading_zeros();
let logs = (a.ilog(2), a.ilog(10));
let count = (!a).count_ones();
let rot = (a << 2) | (a >> 30);

Could be written as:

let [a, b, c]: [u32; 3] = bar();
let div = a.div_ceil(b);
if let Some(quo) = b.checked_div(a) { foo(quo) }
if let Some(decr) = a.checked_sub(1) { foo(decr) }
if let Some(diff) = b.checked_sub(c) { foo(diff) }
if let Some(log) = a.checked_ilog2() { foo(log) }

let single_bit_v1 = a.is_power_of_two();
let single_bit_v2 = a.is_power_of_two();
let log = a.ilog2();
let logs = (a.ilog2(), a.ilog10());
let count = a.count_zeros();
let rot = a.rotate_left(2);

ivan-shrimp avatar Jun 06 '24 09:06 ivan-shrimp

I'm going to start slowly working on this one, probably creating a separate PR for each new lint

@rustbot claim

belyakov-am avatar Jun 10 '24 10:06 belyakov-am

Someone should reopen it, it looks like the "partially fixes #xxx" has (rightly) be understood by GitHub as "fixes #xxx".

samueltardieu avatar Sep 06 '24 07:09 samueltardieu

I think abs_diff is also relevant to this issue. (My WIP lint is here: #14482 )

yotamofek avatar Mar 29 '25 14:03 yotamofek

With the stabilisation of {integer}::midpoint and {integer}::is_multiple_of, I would like to draw attention to this issue again. If this is worked on these too should have lints against manual reïmplementations.

schuelermine avatar May 15 '25 18:05 schuelermine