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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
