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 [manual_ilog2] lint #13331

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Sour1emon
Copy link
Contributor

changelog: new lint: [manual_ilog2]

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

Part of #12894

@rustbot
Copy link
Collaborator

rustbot commented Sep 1, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 1, 2024
@Sour1emon Sour1emon force-pushed the manual_ilog2_lint branch 2 times, most recently from 1de3240 to 9a9bd69 Compare September 1, 2024 20:41
let b: u64 = 543534;
let _ = b.ilog2();

let _ = 64 - b.leading_zeros(); // No lint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it check // No lint

Suppose no need to check this additionally

Copy link
Contributor Author

@Sour1emon Sour1emon Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual ilog2 implementation is bit_width - 1 - x.leading_zeros() so this is just making sure that any other constant besides bit_width - 1 does not lint.

@bors
Copy link
Collaborator

bors commented Sep 3, 2024

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

@bors
Copy link
Collaborator

bors commented Sep 5, 2024

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

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need macro checks. in_external_macro combined with context checks should work (span.ctxt() should be the same for all involved expressions).

Otherwise just a couple of small nits and the test looks like it needs to be re-blessed..

Comment on lines +52 to +55
if !self.msrv.meets(msrvs::MANUAL_ILOG2) {
return;
}
let mut applicability = Applicability::MachineApplicable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to check the HIR tree first.

}

if let ExprKind::MethodCall(method_name, reciever, args, _) = expr.kind
&& method_name.ident.as_str() == "ilog"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string check should be moved to after others. It requires locking the symbol table.

}
}

if let ExprKind::MethodCall(method_name, reciever, args, _) = expr.kind
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use [arg] instead to save the length check later.

&& args.len() == 1
&& let ExprKind::Lit(lit) = args[0].kind
&& let LitKind::Int(Pu128(2), _) = lit.node
&& cx.typeck_results().expr_ty(reciever).is_integral()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a check for an inherent method rather than just a type check (could be a trait function with the same name). You can add is_inherent_method_call.

&& let ExprKind::Lit(lit) = left.kind
&& let LitKind::Int(Pu128(val), _) = lit.node
&& let ExprKind::MethodCall(method_name, reciever, _, _) = right.kind
&& method_name.ident.as_str() == "leading_zeros"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need a check for an inherent method.

@bors
Copy link
Collaborator

bors commented Sep 22, 2024

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

@bors
Copy link
Collaborator

bors commented Oct 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants