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.

Sign up for GitHub

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

Jump to bottom

Add lint named implicit_saturating_sub #5427

Merged
merged 3 commits into from
Apr 18, 2020
Merged

Add lint named implicit_saturating_sub #5427

merged 3 commits into from
Apr 18, 2020

Conversation

pmk21
Copy link
Contributor

@pmk21 pmk21 commented Apr 6, 2020 •
edited by flip1995
Loading

Fixes: #5399
I've made a basic skeleton of the lint, would love more feedback on how to make it better.
changelog: Add lint [implicit_saturating_sub]

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Apr 6, 2020
@flip1995
Copy link
Member

flip1995 commented Apr 6, 2020 •
edited
Loading

It would be good to write more tests for other types. There should be at least one example for every type where the lint should trigger and where it should not trigger. Also there should be tests for more than one expression in a block. Also a test where the the the (bit of a stutter here 😄) binding in the condition is different from the binding that is mutated in the body.

Generally speaking, be creative with testing, write tests for as many edge cases you can think of and the run this lint on. Modify the linting code until only the things, that you think should get linted, are linted.

@pmk21
Copy link
Contributor Author

pmk21 commented Apr 7, 2020

I'm not sure if the lint should be applied on signed integers. And I'm not quite sure how to check the type(signed or unsigned integer) of the variable in the if condition.

@bors
Copy link
Collaborator

bors commented Apr 8, 2020

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

flip1995
flip1995 requested changes Apr 8, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

NIT in test: Can you rename i_XX to u_XX for unsigned integers? This confused me for a bit.

tests/ui/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
@pmk21 pmk21 requested a review from flip1995 April 8, 2020 17:49
@pmk21
Copy link
Contributor Author

pmk21 commented Apr 8, 2020

Should I also be checking for iXX::min_value() in the if condition?

@flip1995
Copy link
Member

flip1995 commented Apr 8, 2020

Yes, this would be good.

@pmk21 pmk21 changed the title [WIP] Add lint named implicit_saturating_sub Add lint named implicit_saturating_sub Apr 9, 2020
@pmk21
Copy link
Contributor Author

pmk21 commented Apr 9, 2020

I think I've handled the symmetric if condition 😄

@pmk21 pmk21 requested a review from flip1995 April 9, 2020 14:57
flip1995
flip1995 requested changes Apr 15, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Tests and impl look really good now!

clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
tests/ui/implicit_saturating_sub.rs Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently.

@pmk21
Copy link
Contributor Author

pmk21 commented Apr 16, 2020

No problems 😃
I guess I have made all requested changes.

@pmk21 pmk21 requested a review from flip1995 April 16, 2020 14:02
flip1995
flip1995 reviewed Apr 16, 2020
View reviewed changes
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
@pmk21
Copy link
Contributor Author

pmk21 commented Apr 17, 2020

I apologize for the unbelievably time consuming PR 😅.

flip1995
flip1995 reviewed Apr 17, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Just some NITs left.

clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
tests/ui/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
clippy_lints/src/implicit_saturating_sub.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

Can you squash some of the commits please? Everything else LGTM.

@phansch
Copy link
Member

phansch commented Apr 18, 2020

@bors r=flip1995

@bors
Copy link
Collaborator

bors commented Apr 18, 2020

📌 Commit 1c11035 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Apr 18, 2020

⌛ Testing commit 1c11035 with merge 891e1a8...

@bors
Copy link
Collaborator

bors commented Apr 18, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 891e1a8 to master...

@bors bors merged commit 891e1a8 into rust-lang:master Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest saturating_sub for if i != 0 { i -= 1; }
4 participants
@pmk21 @flip1995 @bors @phansch