aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:399-400 + Check->diag(MoveArgLoc, + "std::move of the const expression has no effect; " + "remove std::move() or make the variable non-const", + DiagnosticIDs::Note); ---------------- I don't think this diagnostic makes sense on the location of the declaration -- the diagnostic is talking about expressions but the code the user is looking at is a declaration. I think it may make more sense to change the diagnostic at the use location to read `'%0' used after it was moved; move of a 'const' argument has no effect` and then this note can read `variable '%0' declared const here` (so long as you get the correct declaration location -- the lambda example in the test below would be incorrect, for instance). ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:329 // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here + // CHECK-NOTES: [[@LINE-6]]:7: note: std::move of the const expression {{.*}} }; ---------------- zinovy.nis wrote: > Quuxplusone wrote: > > It continues to seem silly to me that you give an extra note here saying > > that line 325 doesn't do anything, when of course line 336 doesn't do > > anything either (and you don't give any extra note there). > > > > This clang-tidy warning isn't supposed to be about what //physically > > happens// in the machine code during any particular compilation run; it's > > supposed to be about helping the user avoid //semantic// bugs by cleaning > > up their codebase's //logical// behavior. The rule is "don't use things > > after moving from them," period. > > > > Analogously, if there were a clang-tidy warning to say "always indent four > > spaces after an `if`," and you proposed to add a note to some cases that > > said "...but here a three-space indent is OK, because C++ is > > whitespace-insensitive" — I'd also find //that// note to be mildly > > objectionable. We want to train people to do the right thing, not to do the > > right thing "except in this case because hey, it doesn't matter to the > > machine." > Thanks for the feedback. I got your point. But my note (may be expressed with > wrong words) is about that there're 2 ways to fix the issue: either get rid > of 'std::move' or somehow remove 'const'. That was the main purpose of my > commit. In this particular instance, I find the new note to be perplexing -- the constant expression being referenced is the original declaration of `a` which is not `const` (the captured `a` within the lambda is what's `const`). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74692/new/ https://reviews.llvm.org/D74692 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits