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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

I'm not at all convinced that any of this complexity is worth it. Have you run this check on any large codebase to see how many false positives it has? Does it catch any true positives?



================
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();
----------------
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."


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:287
+  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 'Tmp'&
+  return std::move(t);
----------------
This suggestion seems bad. If `Tmp&&` was originally a typo, then the machine can't know what the programmer might have meant. If `Tmp&&` was trying to indicate "take ownership of," then the right fix would probably be to pass `Tmp` by value.


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

https://reviews.llvm.org/D107450