[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 27 01:20:39 PDT 2021
whisperity added a comment.
I'm a bit confused about this, but it has been a while since I read this patch. Am I right to think that the code in the patch and the original submission (the patch summary) diverged since the review started? I do not see anything "removed" from the test files, only a new addition. Or did you accidentally upload a wrong diff somewhere? The original intention of the patch was to remove bogus printouts.
In general: the test file ends without any testing for functions with multiple parameters. Are they out-of-scope by design?
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:131-145
auto Diag = diag(FileMoveRange.getBegin(),
"std::move of the %select{|const }0"
- "%select{expression|variable %4}1 "
- "%select{|of the trivially-copyable type %5 }2"
- "has no effect; remove std::move()"
- "%select{| or make the variable non-const}3")
+ "%select{expression|variable %5}1 "
+ "%select{|of the trivially-copyable type %6 }2"
+ "has no effect%select{; remove std::move()||; consider "
+ "changing %7's parameter from %8 to 'const %9 &'}3"
+ "%select{| or make the variable non-const}4")
----------------
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:139
<< IsConstArg << IsVariable << IsTriviallyCopyable
- << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
- << Arg->getType();
+ << (IsRValueReferenceArg + (IsRValueReferenceArg &&
+ IsTriviallyCopyable &&
----------------
**`+`**? Shouldn't this be `||`? I know they're equivalent over Z_2, but if we're using `&&` instead of `*` then let's adhere to the style.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263
+ int a = 10;
+ forwardToShowInt(std::move(a));
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect
+}
----------------
Quuxplusone wrote:
> Sockke wrote:
> > Quuxplusone wrote:
> > > ```
> > > forwardToShowInt(std::move(a));
> > > // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect
> > > ```
> > > I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
> > > It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were `clang -Wall`, then we definitely would //not// want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?
> > > ```
> > > forwardToShowInt(std::move(a));
> > > // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect
> > > ```
> > > I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
> > > It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were `clang -Wall`, then we definitely would //not// want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?
> >
> > Yes, I agree with you. I did not remove warning to maintain the original behavior, which will make the current patch clearer. In any case, it is easy for me to make this modification if you want.
> I'll defer on this subject to whomever you get to review the code change in `MoveConstArgCheck.cpp`. (@whisperity perhaps?)
(Negative, it would be @aaron.ballman or @alexfh or the other maintainers. I'm just trying to give back to the community after taking away //a lot// of reviewer effort with a humongous check that I just recently pushed in after //years// of (re-)development.)
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263
+ int a = 10;
+ forwardToShowInt(std::move(a));
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect
+}
----------------
whisperity wrote:
> Quuxplusone wrote:
> > Sockke wrote:
> > > Quuxplusone wrote:
> > > > ```
> > > > forwardToShowInt(std::move(a));
> > > > // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect
> > > > ```
> > > > I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
> > > > It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were `clang -Wall`, then we definitely would //not// want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?
> > > > ```
> > > > forwardToShowInt(std::move(a));
> > > > // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect
> > > > ```
> > > > I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
> > > > It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were `clang -Wall`, then we definitely would //not// want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?
> > >
> > > Yes, I agree with you. I did not remove warning to maintain the original behavior, which will make the current patch clearer. In any case, it is easy for me to make this modification if you want.
> > I'll defer on this subject to whomever you get to review the code change in `MoveConstArgCheck.cpp`. (@whisperity perhaps?)
> (Negative, it would be @aaron.ballman or @alexfh or the other maintainers. I'm just trying to give back to the community after taking away //a lot// of reviewer effort with a humongous check that I just recently pushed in after //years// of (re-)development.)
>>! In D107450#2958900, @Quuxplusone wrote:
> It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all?
As an individual not representing but myself, I would say no. At least not in a check which is not sold as a //style-like// or //guideline-enforcement// analysis implementation.
> If this were `clang -Wall`, then we definitely would //not// want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?
I think in general tools should have this policy. 😇
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:267-270
+void showTmp(Tmp &&) {}
+void testTmp() {
+ Tmp t;
+ showTmp(std::move(t));
----------------
Is there a test case covering if there are separate invocations of the function? Just to make sure that the //consider changing the parameter// suggestion won't become an annoyance either, and result in conflicts with other parts of the code.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:271
+ showTmp(std::move(t));
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect; consider changing showTmp's parameter from 'Tmp &&' to 'const Tmp &'
+}
----------------
We could ensure that the symbol name is printed the same way as the other placeholder-injected named decls.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:261
+ showInt(std::move(a));
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; consider changing showInt's parameter from 'int'&& to 'int'&
+ return std::move(a);
----------------
MTC wrote:
> Change **'int'&&** -> **'int&&'** and **'int&'** -> **int**.
>
> Make `consider changing showInt's parameter from 'int'&& to 'int'&` as a note instead of a warning. And I don't have a strong preference for the position of the note, but I personally want to put it in the source location of the function definition. and
>>! In D107450#2936824, @MTC wrote:
> but I personally want to put it in the source location of the function definition
(I think you submitted the comment too early, @MTC, as there's an unterminated sentence there.)
But I agree with this. It should be put there when we're talking about the function's signature. The reason being the developer reading the warning could dismiss a potential false positive earlier if they can also immediately read (at least a part of...) the function's signature.
@Sockke you can do this by doing **another** `diag()` call with the `Note` value given as the 3rd parameter. And you can specify the SourceLocation of the affected parameter for this note.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107450/new/
https://reviews.llvm.org/D107450
More information about the cfe-commits
mailing list