Add suggestion for neg_multiply lint #8144
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
Hey @Gh0stm4chine and welcome to the project,
I've made some suggestions on how the code can be improved, the general direction was already good 👍.
One more thing related to the PR description: Clippy creates it's changelog based on the description. Could you include a text like this: changelog: create a suggestion for [`neg_multiply`] or something similar? Otherwise, everything looks good. Let me know if you have any more questions 🙃
clippy_lints/src/neg_multiply.rs
Outdated
| cx, | ||
| NEG_MULTIPLY, | ||
| span, | ||
| "this `multiplication with -1` can be written more succinctly", |
There was a problem hiding this comment.
| "this `multiplication with -1` can be written more succinctly", | |
| "this multiplication with -1 can be written more succinctly", |
It looks strange to me to put "multiplication with -1" in grave accents. Is there a reason for that? I would understand if it were * -1 or something similar that's actually code.
The old string (above) used grave accents only for the number -1 (which can be code):
- span_lint(cx, NEG_MULTIPLY, span, "negation by multiplying with `-1`");Maybe you meant to do that instead, if at all?
There was a problem hiding this comment.
Hi @r00ster91 ,
Yes you are right 👍 I will fix it.
|
It looks good to me 👍. The pipeline failed as it was unable to apply one suggestion. This can happen when multiple suggestions overlap, but that shouldn't be the case here. I've restarted the pipeline to see if it works now. Also thank you to @r00ster91 for the review comment 👍 |
tests/ui/neg_multiply.rs
Outdated
| @@ -1,3 +1,4 @@ | |||
| // run-rustfix | |||
| #![warn(clippy::neg_multiply)] | |||
| #![allow(clippy::no_effect, clippy::unnecessary_operation)] | |||
There was a problem hiding this comment.
| #![allow(clippy::no_effect, clippy::unnecessary_operation)] | |
| #![allow(clippy::no_effect, clippy::unnecessary_operation, clippy::precedence)] |
The clippy::precedence lint triggers for the 0xcafe | 0xff00 * -1; expression. This causes the pipeline to fail. If you allow that lint and rerun the tests and update the output, everything should be fine 🙃
There was a problem hiding this comment.
The pipeline fails because it can't compile the fixed code. I tested it locally, it compiles fine 😕
There was a problem hiding this comment.
Some tests can't be executed on Windows, if you use that it might explain this behavior. I'll check it out and try to find the problem 🙃
There was a problem hiding this comment.
The tests fail due to the configuration of compiletest-rs. In the normal UI tests all lints inside the unused group are allowed, this is sadly not true for .fixed files. The problem can be solved by adding #![allow(unused)] to the file, as that will allow these lints and prevent the warnings that make the test fail.
(This is something I want to fix at some point, but for now, this is the best and easiest solution. Sorry that you ran into this 😅)
There was a problem hiding this comment.
Some tests can't be executed on Windows, if you use that it might explain this behavior. I'll check it out and try to find the problem 🙃
Yes, I am on Windows, but actually running the test locally was failing too. I thought the problem was about compiling the .fixed file.
The tests fail due to the configuration of
compiletest-rs. In the normal UI tests all lints inside theunusedgroup are allowed, this is sadly not true for.fixedfiles. The problem can be solved by adding#![allow(unused)]to the file, as that will allow these lints and prevent the warnings that make the test fail.
I was not sure if the #![allow(unused)] should be added in the .fixed only even though it's a generated file ?
I added it in test file for now.
There was a problem hiding this comment.
Yes, I am on Windows, but actually running the test locally was failing too. I thought the problem was about compiling the .fixed file.
Alright, your assumption was correct, it was with the compilation of the .fixed files.
I was not sure if the
#![allow(unused)]should be added in the .fixed only even though it's a generated file?
I added it in test file for now.
You did it correctly, that's what I meant 👍
|
Looks good to me, thank you for the quick changes 🙃 @bors r+ |
|
📌 Commit 13cc452 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This fixes #8115 by adding a suggestion for [neg_multiply].
My first issue on Github, any feedback or input is welcome 😃
changelog: create a suggestion for
neg_multiply