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