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

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Any thoughts? : )



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263
+  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; consider changing forwardToShowInt's parameter from 'int'&& to 'int'&
+}
----------------
Quuxplusone wrote:
> `forwardToShowInt` takes `T&&`, not `int&&`, and so it can't be changed in the way the diagnostic is suggesting. I think the right answer here is not to emit the diagnostic at all, when the offending function is a template.
> 
> (Relatively minor nits, all moot because this diagnostic should not be emitted at all: `'int'&&` should be `'int&&'`, `trivially copyable` should not be hyphenated, and `int&` is a strictly worse suggestion than either `const int&` or plain `int`.  IMVHO it would be reasonable to land a very trivial patch to remove the hyphen from `trivially copyable` and update the tests, and then rebase this PR on top of that.)
Keep the original diagnostic behavior, but without "remove std::move" message.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263
+  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; consider changing forwardToShowInt's parameter from 'int'&& to 'int'&
+}
----------------
Quuxplusone wrote:
> Sockke wrote:
> > Quuxplusone wrote:
> > > `forwardToShowInt` takes `T&&`, not `int&&`, and so it can't be changed in the way the diagnostic is suggesting. I think the right answer here is not to emit the diagnostic at all, when the offending function is a template.
> > > 
> > > (Relatively minor nits, all moot because this diagnostic should not be emitted at all: `'int'&&` should be `'int&&'`, `trivially copyable` should not be hyphenated, and `int&` is a strictly worse suggestion than either `const int&` or plain `int`.  IMVHO it would be reasonable to land a very trivial patch to remove the hyphen from `trivially copyable` and update the tests, and then rebase this PR on top of that.)
> > Keep the original diagnostic behavior, but without "remove std::move" message.
> Okay, I have no //complaints// about the diagnostics being tested by these tests anymore.
> I am //puzzled// about why the tool is suggesting to rewrite `int&&` as `int`, but not suggesting to rewrite `Tmp&&` as `Tmp`; those cases should be exactly isomorphic, right?
> But the goal of this PR is now to //eliminate wrong fixits//, and I believe you've succeeded at that.
> Btw, I'm not qualified to review the actual code change in `MoveConstArgCheck.cpp`; I'll let someone else do that.
Thank you for your reply! Yes, they should be exactly isomorphic when Tmp is a trivially copyable type.


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

https://reviews.llvm.org/D107450