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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
