whisperity added inline comments.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:261
+ showInt(std::move(a));
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a'
of the trivially-copyable type 'int' has no effect; remove std::move()
+ return std::move(a);
----------------
Quuxplusone wrote:
> whisperity wrote:
> > MTC wrote:
> > > Quuxplusone wrote:
> > > > This warning is incorrect, as indicated in your PR summary. Isn't the
> > > > point of this PR mainly to remove this incorrect warning?
> > > I guess what @Sockke wants to express is that applying `std::move` on the
> > > integer type makes no sense.
> > Yes, but there is no //FixIt// for this warning, but even if the user fixes
> > this //manually// according to the warning, it'll result in
> >
> > > error: cannot bind rvalue reference of type `int&&` to lvalue of type
> > > `int`
> >
> > because `showInt` takes `int&&`!
> >
> > So this is not just unfixable automatically (hence no `CHECK-FIXES` line),
> > it isn't fixable at all.
> At a really high level, yeah, this code is bad code. :) But this is exactly
> Sockke's "case 1":
> > A trivially-copyable object wrapped by std::move is passed to the function
> > with rvalue reference parameters. Removing std::move will cause compilation
> > errors.
> So a warning here would have to be, like, "`std::move` of `a`, of trivially
> copyable type `int`, has no runtime effect; consider changing `showInt`'s
> parameter from `int&&` to `int`."
> Also, should add a test case of the form
> ```
> void showInt(int&&);
> template<class T> void forwardToShowInt(T&& t) {
> showInt(static_cast<T&&>(t)); }
> void test() {
> int a = 10;
> forwardToShowInt(std::move(a));
> }
> ```
> to make sure the diagnostic (or lack thereof) plays well with templates.
> Here, refactoring `forwardToShowInt`'s parameter from `T&&` to `T` is
> unlikely to be a realistic option.
Yes, I was thinking the same, maybe it could warn about the function (in a note
or something). But then we reach the point that the check's name is //fuzzy//
at best... (It has an already outdated name, though.)
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
