Sockke added a comment. Any thoughts? : )
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263 + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; consider changing forwardToShowInt's parameter from 'int'&& to 'int'& +} ---------------- Quuxplusone wrote: > `forwardToShowInt` takes `T&&`, not `int&&`, and so it can't be changed in > the way the diagnostic is suggesting. I think the right answer here is not to > emit the diagnostic at all, when the offending function is a template. > > (Relatively minor nits, all moot because this diagnostic should not be > emitted at all: `'int'&&` should be `'int&&'`, `trivially copyable` should > not be hyphenated, and `int&` is a strictly worse suggestion than either > `const int&` or plain `int`. IMVHO it would be reasonable to land a very > trivial patch to remove the hyphen from `trivially copyable` and update the > tests, and then rebase this PR on top of that.) Keep the original diagnostic behavior, but without "remove std::move" message. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263 + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; consider changing forwardToShowInt's parameter from 'int'&& to 'int'& +} ---------------- Quuxplusone wrote: > Sockke wrote: > > Quuxplusone wrote: > > > `forwardToShowInt` takes `T&&`, not `int&&`, and so it can't be changed > > > in the way the diagnostic is suggesting. I think the right answer here is > > > not to emit the diagnostic at all, when the offending function is a > > > template. > > > > > > (Relatively minor nits, all moot because this diagnostic should not be > > > emitted at all: `'int'&&` should be `'int&&'`, `trivially copyable` > > > should not be hyphenated, and `int&` is a strictly worse suggestion than > > > either `const int&` or plain `int`. IMVHO it would be reasonable to land > > > a very trivial patch to remove the hyphen from `trivially copyable` and > > > update the tests, and then rebase this PR on top of that.) > > Keep the original diagnostic behavior, but without "remove std::move" > > message. > Okay, I have no //complaints// about the diagnostics being tested by these > tests anymore. > I am //puzzled// about why the tool is suggesting to rewrite `int&&` as > `int`, but not suggesting to rewrite `Tmp&&` as `Tmp`; those cases should be > exactly isomorphic, right? > But the goal of this PR is now to //eliminate wrong fixits//, and I believe > you've succeeded at that. > Btw, I'm not qualified to review the actual code change in > `MoveConstArgCheck.cpp`; I'll let someone else do that. Thank you for your reply! Yes, they should be exactly isomorphic when Tmp is a trivially copyable type. 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
