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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:254
+  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 'const int &'
+}
----------------
>From `int &`? Isn't the parameter `int &&` there?


================
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));
----------------
Sockke wrote:
> 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？
> 
Yes, that is what I mean. But I would like to see a test case where the "target function" is called **multiple times** with different arguments. One call with `std::move(x)`, one with a direct temporary, etc. So we can make sure that if a function is called more than 1 time, we can still be sure there won't be bad warnings or bad fixits that contradict each other.


================
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);
----------------
Sockke wrote:
> 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.
I think it is okay if the additional note for this is added in this patch, there is no need for a separate patch on that. It helps understand WHY the FixIt is wrong or not wrong, anyway.


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

https://reviews.llvm.org/D107450