[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke marked an inline comment as not done.
Sockke added a comment.

Thanks for your review, I greatly appreciate it. @whisperity
There is no wrong case in the original test file, which is why we did not catch the bug in the test suite. From this, I added some new cases. 
I tested the functions with multi-parameters, there is no problem, I will add them to the test file.

In D107450#2968708 <https://reviews.llvm.org/D107450#2968708>, @whisperity wrote:

> 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")
----------------
whisperity wrote:
> 
> 




================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:139
                 << IsConstArg << IsVariable << IsTriviallyCopyable
-                << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
-                << Arg->getType();
+                << (IsRValueReferenceArg + (IsRValueReferenceArg &&
+                                            IsTriviallyCopyable &&
----------------
whisperity wrote:
> **`+`**? 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.
> **`+`**? 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.

The reason for this is that the maximum capacity of diag is 10.


================
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));
----------------
whisperity wrote:
> 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.
The warning will only be given when calling this function and passing in the parameters wrapped with std::move. Is my understanding correct？



================
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 &'
+}
----------------
whisperity wrote:
> We could ensure that the symbol name is printed the same way as the other placeholder-injected named decls.
Yes,  this is a better implementation.


================
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);
----------------
whisperity wrote:
> 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.
> >>! 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.

Yes, it is reasonable to add a note, but would you mind adding this modification to the patch that fixes AutoFix? If you don't mind, I will update it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107450/new/

https://reviews.llvm.org/D107450