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_rem_euclid] lint #9031

Merged
merged 10 commits into from
Jun 24, 2022
Merged

Conversation

evantypanski
Copy link
Contributor

Closes #8883

Adds a lint for checking manual use of rem_euclid(n)

changelog: Add [manual_rem_euclid] lint

@rust-highfive
Copy link

r? @Jarcho

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 21, 2022
return None;
};

if int_const > FullInt::S(0) {
Copy link

@ghost ghost Jun 22, 2022

Choose a reason for hiding this comment

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

I would rewrite this to not have the unwrap(). Something like

let val = match int_const {
    FullInt::S(s) => s.try_into(),
    FullInt::U(u) => Some(u)
}
Some((val?, other_op))

There might be a better way of doing it.

--

Edit: Just a comment. Not a review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, believe this is in the spirit of that 75ed0c9

Copy link
Contributor

Choose a reason for hiding this comment

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

You can match on the constant and skip the bounds check here. i.e.

match x {
    S(x) => x.try_into().ok(),
    U(x) => Some(x),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, realized you can't do x % 0 because zero divisor is invalid, so the equal checks for the consts makes sure the const is nonzero so some checks were completely unnecessary, thanks

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.

rem_euclid is only const as of 1.52. You'll need to add a call to clippy_utils::in_constant with a second msrv check.

clippy_lints/src/manual_rem_euclid.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_rem_euclid.rs Outdated Show resolved Hide resolved
@Jarcho
Copy link
Contributor

Jarcho commented Jun 23, 2022

There should be a call to in_external_macro, and an msrv test for the const version with the msrv set between the two versions. Otherwise it all looks good.

@Jarcho
Copy link
Contributor

Jarcho commented Jun 24, 2022

Looks good now. Thank you.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 24, 2022

📌 Commit df26c3f has been approved by Jarcho

@bors
Copy link
Collaborator

bors commented Jun 24, 2022

⌛ Testing commit df26c3f with merge e17864e...

@bors
Copy link
Collaborator

bors commented Jun 24, 2022

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

@bors bors merged commit e17864e into rust-lang:master Jun 24, 2022
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.

New lint: manual reimplementation of .rem_euclid()
4 participants