Skip to content

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Aug 23, 2022

Fixes #9307 and makes the arithmetic lint behave like integer_arithmetic.

It is worth noting that literal integers of a binary operation (1 + 1, i32::MAX + 1), regardless if they are in a constant environment, won't trigger the lint. Assign operations also have similar reasoning.

changelog: Consider literals in the arithmetic lint

@rust-highfive
Copy link

r? @Alexendoo

(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 Aug 23, 2022
@@ -1,4 +1,4 @@
#![warn(clippy::integer_arithmetic, clippy::float_arithmetic)]
#![warn(clippy::arithmetic)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to show that arithmetic behaves like integer_arithmetic and float_arithemtic. Once approved, I will revert this line.

Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

👍

Just a couple small bits to suggest

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 1, 2022

Addressed all comments with a new strategy and additional tests

@c410-f3r c410-f3r force-pushed the arith branch 2 times, most recently from 8f91440 to 871b75f Compare September 1, 2022 22:56
Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

I think it'd be worth updating the lint documentation to mention that the lint doesn't fire for cases where overflow can't happen

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 7, 2022

Nice, thanks!

I think it'd be worth updating the lint documentation to mention that the lint doesn't fire for cases where overflow can't happen

Done

@c410-f3r c410-f3r force-pushed the arith branch 2 times, most recently from 3a3e315 to fca2624 Compare September 7, 2022 12:20
@Alexendoo
Copy link
Member

Thanks!

@bors r+

Renaming the lint may be a good idea yeah. Could ask on zulip for some input on a potential name, my vote would probably be the slight variation arithmetic_side_effects

@bors
Copy link
Contributor

bors commented Sep 7, 2022

📌 Commit 0d078c9 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 7, 2022

⌛ Testing commit 0d078c9 with merge 617417e...

@bors
Copy link
Contributor

bors commented Sep 7, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 617417e to master...

@bors bors merged commit 617417e into rust-lang:master Sep 7, 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.

Allowing clippy::arithmetic in const contexts
4 participants