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

Trigger modulo_one lint also on -1. #6360

Merged
merged 3 commits into from
Nov 28, 2020
Merged

Trigger modulo_one lint also on -1. #6360

merged 3 commits into from
Nov 28, 2020

Conversation

mlegner
Copy link
Contributor

@mlegner mlegner commented Nov 21, 2020

Fixes #6321.

changelog: trigger modulo_one lint also on -1

@rust-highfive
Copy link

r? @llogiq

(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 21, 2020
@mlegner
Copy link
Contributor Author

mlegner commented Nov 21, 2020

Question about tests with static constants: Is there a good reason why the lint should not be triggered by those or is this a bug?
If it is a bug, I could either try to fix it in this PR or create an issue for it.

@llogiq
Copy link
Contributor

llogiq commented Nov 21, 2020

I don't know the reason. At least filing an issue sounds like a good idea.

@mlegner
Copy link
Contributor Author

mlegner commented Nov 22, 2020

I don't know the reason. At least filing an issue sounds like a good idea.

In general, there seems to be quite some inconsistency with constant/literal testing in clippy. For example, the integer_arithmetic lint also tests for literals but does not do constant folding at all. I will suggest consolidating these to rely on a single constant-comparison function similar to / replacing the is_integer_const function.

@llogiq
Copy link
Contributor

llogiq commented Nov 22, 2020

This fails the dogfood test. There's a function in misc.rs with 110 lines. We should probably factor some of the lines out into another function.

@mlegner
Copy link
Contributor Author

mlegner commented Nov 23, 2020

This fails the dogfood test. There's a function in misc.rs with 110 lines. We should probably factor some of the lines out into another function.

I have created a check_binary function that is called in the check_expr function.

@bors
Copy link
Contributor

bors commented Nov 27, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@llogiq
Copy link
Contributor

llogiq commented Nov 28, 2020

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2020

📌 Commit b822632 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Nov 28, 2020

⌛ Testing commit b822632 with merge a644930...

@bors
Copy link
Contributor

bors commented Nov 28, 2020

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

@bors bors merged commit a644930 into rust-lang:master Nov 28, 2020
@mlegner mlegner deleted the mod_one branch November 28, 2020 12:40
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.

Lint for calculating remainder with divisor of -1
4 participants