Skip to content

Add lint named implicit_saturating_sub - #5427

Merged
bors merged 3 commits into
rust-lang:masterfrom
pmk21:implicit-sat-sub
Apr 18, 2020
Merged

Add lint named implicit_saturating_sub#5427
bors merged 3 commits into
rust-lang:masterfrom
pmk21:implicit-sat-sub

Conversation

@pmk21

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

Copy link
Copy Markdown
Contributor

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

flip1995 commented Apr 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

pmk21 commented Apr 7, 2020

Copy link
Copy Markdown
Contributor Author

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

bors commented Apr 8, 2020

Copy link
Copy Markdown
Contributor

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

@pmk21
pmk21 force-pushed the implicit-sat-sub branch from 471d138 to b267deb Compare April 8, 2020 14:48
flip1995
flip1995 requested changes Apr 8, 2020
View reviewed changes

@flip1995 flip1995 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread tests/ui/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
@pmk21
pmk21 force-pushed the implicit-sat-sub branch from f295538 to 6bbc5aa Compare April 8, 2020 17:47
@pmk21
pmk21 requested a review from flip1995 April 8, 2020 17:49
@pmk21

pmk21 commented Apr 8, 2020

Copy link
Copy Markdown
Contributor Author

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

@flip1995

flip1995 commented Apr 8, 2020

Copy link
Copy Markdown
Member

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
flip1995
flip1995 reviewed Apr 9, 2020
View reviewed changes
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
@pmk21

pmk21 commented Apr 9, 2020

Copy link
Copy Markdown
Contributor Author

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

@flip1995 flip1995 left a comment

Copy link
Copy Markdown
Member

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!

Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread tests/ui/implicit_saturating_sub.rs
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
@flip1995

flip1995 commented Apr 15, 2020

Copy link
Copy Markdown
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
pmk21 force-pushed the implicit-sat-sub branch from aefab0e to 14cd457 Compare April 16, 2020 13:59
@pmk21

pmk21 commented Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

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
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
@pmk21
pmk21 force-pushed the implicit-sat-sub branch from 14cd457 to 90e8edb Compare April 17, 2020 19:01
@pmk21

pmk21 commented Apr 17, 2020

Copy link
Copy Markdown
Contributor Author

I apologize for the unbelievably time consuming PR 😅.

flip1995
flip1995 reviewed Apr 17, 2020
View reviewed changes

@flip1995 flip1995 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just some NITs left.

Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread tests/ui/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
Comment thread clippy_lints/src/implicit_saturating_sub.rs Outdated
@flip1995

flip1995 commented Apr 17, 2020

Copy link
Copy Markdown
Member

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

@pmk21
pmk21 force-pushed the implicit-sat-sub branch from 73a6440 to 1c11035 Compare April 18, 2020 06:37
@phansch

phansch commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

@bors r=flip1995

@bors

bors commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

📌 Commit 1c11035 has been approved by flip1995

@bors

bors commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

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

@bors

bors commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

☀️ 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