aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:145
+ if ((!ReceivingCallExpr ||
+ ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) &&
+ (!ReceivingConstructExpr ||
----------------
Not all `CallExpr` objects have a direct callee, so this will crash (such as
calls through a function pointer rather than a direct function call). It may be
worth adding test coverage for this.
================
Comment at:
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:149-153
+ std::string FunctionName;
+ if (ReceivingCallExpr)
+ FunctionName = ReceivingCallExpr->getDirectCallee()->getNameAsString();
+ else
+ FunctionName =
ReceivingConstructExpr->getConstructor()->getNameAsString();
----------------
Rather than doing this, you should be able to just pass the `NamedDecl *` in to
the call to `Diag` below and it will be properly formatted and quoted.
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:154
+ FunctionName =
ReceivingConstructExpr->getConstructor()->getNameAsString();
+ auto NoRefType = InvocationParm->getType()->getPointeeType();
+ PrintingPolicy PolicyWithSupressedTag(getLangOpts());
----------------
Please spell the type out in the declaration.
================
Comment at:
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:171-173
} else if (ReceivingExpr) {
+ if (InvocationParm->getOriginalType()->isRValueReferenceType())
+ return;
----------------
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:118
+
+ Removed wrong FixIt for trivially-copyable objects wrapped by
``std::move()`` and passed to an rvalue reference parameter. Removal of
``std::move()`` would break the code.
+
----------------
Can you re-flow this to 80 cols too?
================
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:
> 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. 😇
> 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.
And your review efforts are very much seen and appreciated, @whisperity! Thank
you for the excellent help!
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
