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