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.

Sign up for GitHub

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

Jump to bottom

[arithmetic-side-effects] Consider negative numbers and add more tests #9867

Closed
wants to merge 1 commit into from
Closed

[arithmetic-side-effects] Consider negative numbers and add more tests #9867

wants to merge 1 commit into from

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Nov 18, 2022 •
edited by xFrednet
Loading

Depends on #9840 which depends on #9592.

Adds more tests from integer_arithmetic and also considers more corner-cases.

changelog: PF: [`arithmetic_side_effects]: No longer lints on corner cases with negative number literals
#9867

@rust-highfive
Copy link

r? @flip1995

(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 Nov 18, 2022
@xFrednet
Copy link
Member

Hey @c410-f3r thank you for the pull request. Could you clarify the changelog entry a bit? Does "consider" means it lints cases with negative numbers or does it avoid FPs related to them? If so, which false positive. Thank you in advance!

@flip1995 flip1995 added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 18, 2022
@flip1995
Copy link
Member

Marking as blocked, until other PRs are merged. Please ping me once that is done, so that I don't miss it.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Nov 18, 2022 •
edited
Loading

Hey @c410-f3r thank you for the pull request. Could you clarify the changelog entry a bit? Does "consider" means it lints cases with negative numbers or does it avoid FPs related to them? If so, which false positive. Thank you in advance!

The entry has been updated. Let me know if it is good enough.

@xFrednet
Copy link
Member

Yes, that should be sufficient, thank you!

@bors
Copy link
Collaborator

bors commented Nov 20, 2022

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

@flip1995
Copy link
Member

You will need to rebase on master. The Conflicts are probably not real conflicts. If you rebase with git rebase -i and drop the commits that were already merged, rebase might get through without any conflicts.

#9840 still needs to be merged AFAICT?

@c410-f3r
Copy link
Contributor Author

Yeap, I will rebase once #9840 is merged

@c410-f3r
Copy link
Contributor Author

ping @flip1995

@c410-f3r
Copy link
Contributor Author

@flip1995 I can try another reviewer if you don't have time

@c410-f3r c410-f3r closed this Dec 23, 2022
@c410-f3r c410-f3r mentioned this pull request Dec 23, 2022
Merged
bors added a commit that referenced this pull request Jan 6, 2023
@bors
Auto merge of #10112 - c410-f3r:arith, r=xFrednet
3f0d0c1 
[arithmetic-side-effects] Consider negative numbers and add more tests

Same as #9867.

Opening again because it is not possible to randomly choose a reviewer in an ongoing PR like in the rust repo.

---

changelog: PF: [`arithmetic_side_effects`]: No longer lints on corner cases with negative number literals
[#9867](#9867)
<!-- changelog_checked -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@flip1995 flip1995

Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@c410-f3r @rust-highfive @xFrednet @flip1995 @bors