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

Add lints against more manual integer ops where direct methods exist #12894

Open
1 of 10 tasks
ivan-shrimp opened this issue Jun 6, 2024 · 2 comments · Fixed by #12987
Open
1 of 10 tasks

Add lints against more manual integer ops where direct methods exist #12894

ivan-shrimp opened this issue Jun 6, 2024 · 2 comments · Fixed by #12987
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@ivan-shrimp
Copy link

ivan-shrimp commented Jun 6, 2024

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.

(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 ivan-shrimp added the A-lint Area: New lints label Jun 6, 2024
@Alexendoo Alexendoo added the good-first-issue These issues are a good way to get started with Clippy label Jun 6, 2024
@Alexendoo Alexendoo changed the title Add lint against more manual integer ops where direct methods exist Add lints against more manual integer ops where direct methods exist Jun 6, 2024
@belyakov-am
Copy link
Contributor

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

@rustbot claim

@samueltardieu
Copy link
Contributor

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

@y21 y21 reopened this Sep 6, 2024
bors added a commit that referenced this issue Sep 6, 2024
Add new lint `manual_is_power_of_two`

Suggest using `is_power_of_two()` instead of the manual implementations for some basic cases

Part of #12894

----

changelog: new [`manual_is_power_of_two`] lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints 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.

5 participants