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

[Aaron Ballman via Phabricator via cfe-commits]

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