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

Rename integer_arithmetic #10674

Merged
merged 1 commit into from
May 18, 2023
Merged

Rename integer_arithmetic #10674

merged 1 commit into from
May 18, 2023

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Apr 20, 2023

The lack of official feedback in #10200 made me give up on pursuing the matter but after yet another use-case that is not handled by integer_arithmetic (#10615), I think it is worth trying again.


changelog: Move/Deprecation: Rename integer_arithmetic to arithmetic_side_effects
#10674

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2023

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 20, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Apr 23, 2023

I'll look into this this week. Just as an initial comment it looks like this would be better as a rename rather than deprecation.

@Jarcho
Copy link
Contributor

Jarcho commented May 5, 2023

Sorry, this took longer to get to than expected.

I'm in favour of the change in general. The whole point of integer_arithmetic is to prevent silent edge case errors, and other types can definitely have the same problems as the builtin types. Given that this is a restriction lint, anybody using it should be willing to put up with the annoyance of having to configure it to allow operations which are safe to use.

It might be worth having an attribute which can be applied to the trait impls to opt-out of the lint. I don't feel like this needs to be a blocker, just a thing to consider.

An alternative to deprecating it is to just use the implementation of arithmetic_side_effects, but check the types of both operands before issuing the lint to determine whether integer_arithmetic or arithmetic_side_effects should be used. I'm not convinced the split is worth having though.

cc @rust-lang/clippy if anyone has something to add.

@xFrednet
Copy link
Member

xFrednet commented May 5, 2023

I agree that the merge makes sense, if arithmetic_side_effects covers all cases that integer_arithmetic has :)

@Manishearth
Copy link
Member

I'm in favor

@Jarcho
Copy link
Contributor

Jarcho commented May 10, 2023

@c410-f3r We are good to merge once the lint deprecation is changed to a lint rename and all tests are passing.

@c410-f3r c410-f3r changed the title Deprecate integer_arithmetic Rename integer_arithmetic May 10, 2023
@c410-f3r
Copy link
Contributor Author

Thank your very much @Jarcho and @rust-lang/clippy for accepting this proposal. A lot of effort was put into arithmetic_side_effects and more development will be applied in future iterations.

@Jarcho
Copy link
Contributor

Jarcho commented May 18, 2023

Thank you for pushing this through. @bors r+

@bors
Copy link
Collaborator

bors commented May 18, 2023

📌 Commit 493b2ae has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 18, 2023

⌛ Testing commit 493b2ae with merge 270cbeb...

@bors
Copy link
Collaborator

bors commented May 18, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 270cbeb to master...

@bors bors merged commit 270cbeb into rust-lang:master May 18, 2023
@c410-f3r
Copy link
Contributor Author

Thank you for pushing this through. @bors r+

Thank you!

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