[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Aaron Ballman via Phabricator via cfe-commits]

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