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

[1/N] Implement Arithmetic lint #8905

Merged
merged 1 commit into from
Jun 2, 2022
Merged

[1/N] Implement Arithmetic lint #8905

merged 1 commit into from
Jun 2, 2022

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented May 27, 2022

Assuming that #8903 is OK, this PR starts the creation of the Arithmetic lint with configurable types.

My current struggle to get a rustc review inspired me to create smaller PRs in order to easy review and make merges as fast as possible. So the first step here only moves the arithmetic.rs file to numeric_arithmetic.rs to make room for the new lint.

--
changelog: none

@rust-highfive
Copy link

r? @camsteffen

(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 May 27, 2022
@Jarcho
Copy link
Contributor

Jarcho commented Jun 1, 2022

There's no need to split PR's up this much. There isn't much of a reason to accept the change as it stands. As part of a PR adding a lint for any use of an arithmetic operator the change is perfectly fine. Feel free to keep adding to this PR rather than closing it.

Side note the changes you have here will be broken by #8921 when it gets merged.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jun 1, 2022

There's no need to split PR's up this much. There isn't much of a reason to accept the change as it stands. As part of a PR adding a lint for any use of an arithmetic operator the change is perfectly fine. Feel free to keep adding to this PR rather than closing it.

Side note the changes you have here will be broken by #8921 when it gets merged.

There are PRs sitting idle for months and if the Clippy team couldn't merge 9 diff lines in 5 days, my perception is that a full PR would take much, much, much, much longer.

Perhaps another reviewer?

r? rust-lang/rust-clippy (Blah, doesn't work)

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jun 1, 2022

r? @llogiq

@rust-highfive rust-highfive assigned llogiq and unassigned camsteffen Jun 1, 2022
@llogiq
Copy link
Contributor

llogiq commented Jun 2, 2022

Ok. Feel free to make the next PR bigger and directly ask for my review.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2022

📌 Commit 4fc0ee6 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Jun 2, 2022

⌛ Testing commit 4fc0ee6 with merge 9428e2e...

@bors
Copy link
Contributor

bors commented Jun 2, 2022

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

@bors bors merged commit 9428e2e into rust-lang:master Jun 2, 2022
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jun 2, 2022

Thank you @llogiq

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.

6 participants