Add new lint `manual_is_power_of_two` #13327

Sour1emon · 2024-08-31T23:44:54Z

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

changelog: new [manual_is_power_of_two] lint

rustbot · 2024-08-31T23:44:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

clippy_lints/src/manual_is_power_of_two.rs

bors · 2024-09-03T17:46:27Z

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

alex-semenyuk · 2024-09-05T11:02:28Z

clippy_lints/src/manual_is_power_of_two.rs

+            && let ExprKind::Lit(lit1) = right.kind
+            && let LitKind::Int(Pu128(0), _) = lit1.node
+        {
+            let snippet = snippet_with_applicability(cx, left1.span, "..", &mut applicability);


Might make sense combine sugg and span_lint_and_sugg here and 48-58 so change in one place if needed

alex-semenyuk · 2024-09-05T11:10:20Z

tests/ui/manual_is_power_of_two.fixed

+
+    let b = 4_i64;
+
+    // is_power_of_two only works for unsigned integers


Might be good idea mentioned about this at lint doc as well

llogiq

There is some duplication and the binary operations might be reversed. Either you implement the tests for both directions or you document the missing cases in the lint docs under # Known Problems and we can open an issue to amend the lint later. Both are fine.

clippy_lints/src/manual_is_power_of_two.rs

tests/ui/manual_is_power_of_two.rs

bors · 2024-09-05T23:29:28Z

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

rustbot · 2024-09-06T01:24:34Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

a29f0c4

Co-authored-by: Ruby Lazuli <general@patchmixolydic.com>

llogiq · 2024-09-06T10:33:39Z

clippy_lints/src/manual_is_power_of_two.rs

+            if let ExprKind::MethodCall(method_name, reciever, _, _) = left.kind
+                && method_name.ident.as_str() == "count_ones"
+                && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind()
+                && check_lit(right, 1)
+            {
+                build_sugg(cx, expr, reciever, &mut applicability);
+            }
+
+            // 1 == a.count_ones()
+            if let ExprKind::MethodCall(method_name, reciever, _, _) = right.kind
+                && method_name.ident.as_str() == "count_ones"
+                && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind()
+                && check_lit(left, 1)
+            {
+                build_sugg(cx, expr, reciever, &mut applicability);
+            }


You can loop over [(left, right), (right, left)] to reduce the code duplication here and elsewhere.

llogiq · 2024-09-06T10:35:50Z

Apart from a good deal of code duplication, this seems good to merge, we can refactor in a followup PR later.

@bors r+

bors · 2024-09-06T10:35:52Z

📌 Commit f994797 has been approved by llogiq

It is now in the queue for this repository.

bors · 2024-09-06T10:37:00Z

⌛ Testing commit f994797 with merge fb9913e...

bors · 2024-09-06T10:48:45Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing fb9913e to master...

rustbot assigned llogiq Aug 31, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 31, 2024

Alexendoo mentioned this pull request Sep 1, 2024

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

Open

10 tasks

PatchMixolydic reviewed Sep 2, 2024

View reviewed changes

clippy_lints/src/manual_is_power_of_two.rs Outdated Show resolved Hide resolved

clippy_lints/src/manual_is_power_of_two.rs Outdated Show resolved Hide resolved

Sour1emon force-pushed the master branch from 434ba31 to 28b6d7f Compare September 4, 2024 00:43

alex-semenyuk reviewed Sep 5, 2024

View reviewed changes

llogiq reviewed Sep 5, 2024

View reviewed changes

clippy_lints/src/manual_is_power_of_two.rs Outdated Show resolved Hide resolved

tests/ui/manual_is_power_of_two.rs Show resolved Hide resolved

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 6, 2024

Sour1emon force-pushed the master branch from a29f0c4 to 348a7f8 Compare September 6, 2024 01:35

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 6, 2024

Sour1emon and others added 4 commits September 5, 2024 18:38

Add manual_is_power_of_two

d7996da

Improve "Why this is bad" section

5515566

Co-authored-by: Ruby Lazuli <general@patchmixolydic.com>

Fixed extra backtick

6ecb48f

Co-authored-by: Ruby Lazuli <general@patchmixolydic.com>

Add support for different orders of expression

f994797

Sour1emon force-pushed the master branch from 348a7f8 to f994797 Compare September 6, 2024 01:38

llogiq reviewed Sep 6, 2024

View reviewed changes

bors merged commit fb9913e into rust-lang:master Sep 6, 2024
11 checks passed

Sour1emon deleted the master branch September 7, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new lint `manual_is_power_of_two` #13327

Add new lint `manual_is_power_of_two` #13327

Sour1emon commented Aug 31, 2024 •

edited by llogiq

Loading

rustbot commented Aug 31, 2024

bors commented Sep 3, 2024

alex-semenyuk Sep 5, 2024

alex-semenyuk Sep 5, 2024

llogiq left a comment

bors commented Sep 5, 2024

rustbot commented Sep 6, 2024

llogiq Sep 6, 2024

llogiq commented Sep 6, 2024

bors commented Sep 6, 2024

bors commented Sep 6, 2024

bors commented Sep 6, 2024


		let b = 4_i64;

		// is_power_of_two only works for unsigned integers

Add new lint manual_is_power_of_two #13327

Add new lint manual_is_power_of_two #13327

Conversation

Sour1emon commented Aug 31, 2024 • edited by llogiq Loading

rustbot commented Aug 31, 2024

bors commented Sep 3, 2024

alex-semenyuk Sep 5, 2024

Choose a reason for hiding this comment

alex-semenyuk Sep 5, 2024

Choose a reason for hiding this comment

llogiq left a comment

Choose a reason for hiding this comment

bors commented Sep 5, 2024

rustbot commented Sep 6, 2024

llogiq Sep 6, 2024

Choose a reason for hiding this comment

llogiq commented Sep 6, 2024

bors commented Sep 6, 2024

bors commented Sep 6, 2024

bors commented Sep 6, 2024

Add new lint `manual_is_power_of_two` #13327

Add new lint `manual_is_power_of_two` #13327

Sour1emon commented Aug 31, 2024 •

edited by llogiq

Loading