[PATCH] D107450: [clang-tidy] Fix wrong and missing warnings in performance-move-const-arg

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.

I think we're a bit off track,  and @Sockke wants to accomplish more than one goal in the same patch. I have summarized what we are currently discussing as follow shows:

1. Fix the wrong AutoFix which blocks the compilation.
2. Find more 'unrecommended' std::move usage and give correct warning messages.
3. Whether template should be taken into account.

In addition, I would like to mention that we need to ensure that this check should be consistent with `-Wpessimizing-move`, see https://reviews.llvm.org/D7633, which has done the perfect job.

I suggest that this patch be divided into two patches. In the current patch, fix the **wrong AutoFix**. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:284
+  Tmp t2 = std::move(Tmp());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the expression of the trivially-copyable type 'Tmp' has no effect; remove std::move()
+  // CHECK-FIXES: Tmp t2 = Tmp();
----------------
Quuxplusone wrote:
> Here, the more immediate red flag is that the programmer is using `std::move` on an rvalue expression.
> ```
> std::string&& a();
> std::string& b();
> std::string c();
> auto aa = std::move(a());  // not great, moving an xvalue
> auto bb = std::move(b());  // OK, moving an lvalue
> auto cc = std::move(c());  // not great, moving a prvalue
> ```
> However, the more complicated and guess-worky these diagnostics get, the more I want to see how they interact with templates and generic code.
> ```
> template<class F>
> auto w(F f) {
>     return std::move(f());
> }
> std::string callw1() { std::string s; return w([&]() -> std::string { return s; });
> std::string callw2() { std::string s; return w([&]() -> std::string& { return s; });
> ```
> `callw1` ends up applying `std::move` to a prvalue inside `w`, but when `callw2` calls `w`, the `std::move` actually does something. Possible strategies here include "Warn on code that's only sometimes correct," "Don't warn on code that's only sometimes wrong," and "Don't implement the diagnostic at all because it's too icky."
Agree! For `Tmp t2 = std::move(Tmp());`, what should be reported here is moving a **prvalue**. However, the original behavior is emitting this message.

And the original matcher will ignore template instantiation, see `unless(isInTemplateInstantiation())`. 


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

https://reviews.llvm.org/D107450