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