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

new implicit_saturating_add lint #9549

Merged
merged 1 commit into from
Oct 3, 2022
Merged

new implicit_saturating_add lint #9549

merged 1 commit into from
Oct 3, 2022

Conversation

roynrishingha
Copy link
Contributor

@roynrishingha roynrishingha commented Sep 28, 2022

This fixes #9393

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

changelog: add [manual_saturating_add] lint

@rust-highfive
Copy link

r? @giraffate

(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 Sep 28, 2022
@roynrishingha
Copy link
Contributor Author

r?@llogiq

@rust-highfive rust-highfive assigned llogiq and unassigned giraffate Sep 28, 2022
@roynrishingha roynrishingha changed the title new manual_saturating_add lint new implicit_saturating_add lint Sep 30, 2022
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This already looks pretty good. We best make sure to only lint when nothing is in the else branch of the if expression, otherwise this should be mergeable.

if u8::MAX > a {
b += 1;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see tests against additional.statements in either then or else branch (noting that this led to false positives in the implicit-saturating-sub lint).

@roynrishingha roynrishingha marked this pull request as ready for review October 2, 2022 16:49
@llogiq
Copy link
Contributor

llogiq commented Oct 2, 2022

I think you'll need to rebase on current master before CI will run.

@llogiq
Copy link
Contributor

llogiq commented Oct 3, 2022

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2022

📌 Commit f1c831a has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 3, 2022

⌛ Testing commit f1c831a with merge 09e6c23...

@bors
Copy link
Contributor

bors commented Oct 3, 2022

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

@bors bors merged commit 09e6c23 into rust-lang:master Oct 3, 2022
@roynrishingha roynrishingha deleted the manual_saturating_add branch April 20, 2023 16:03
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.

Add lint IMPLICIT_SATURATING_ADD
5 participants