sammccall added a comment.
This is a subtle and complicated check I'm not so familiar with. I have some
ideas but I'm not confident in the suggestions or if we're missing something.
@mboehme is the expert on this check, he's OOO this week/next week. I'm happy
to muddle along or we can wait for him to take a look :-)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:404
+
+ auto BaseMoveMatcher =
callExpr(callee(functionDecl(hasName("::std::move"))),
argumentCountIs(1),
----------------
Hmm, I think the original name (`CallMoveMatcher`) was good as it matches the
actual call.
I'd probably name the lambda one LambdaWithMoveInitMatcher and the other...
actually it's very close to CallMoveMatcher itself, maybe just inline it?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:416
+ auto CallMoveMatcherLambda = lambdaExpr(
+ forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
----------------
do we want to bind the lambda itself as `moving-call`?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:416
+ auto CallMoveMatcherLambda = lambdaExpr(
+ forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
----------------
sammccall wrote:
> do we want to bind the lambda itself as `moving-call`?
we should probably have a comment explaining *why* lambdas get handled
specially.
If I understand right:
- the normal matcher would already match
- but either MovingCall or ContainingLambda (which?) point at unhelpful nodes
and
- we end up doing the analysis inside the lambda rather than in the enclosing
function
- so never find the following use
(I wonder if it's possible to fix this slightly more directly by tweaking the
MovingCall or ContainingLambda logic)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:418
+ forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+ hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))))))),
+ AncestorMatcher);
----------------
This only matches when the initializer is exactly `std::move(x)`.
Not e.g. if it's `SomeType(std::move(x))`.
Former is probably the common case, but is this deliberate? If we're not sure
exactly which cases are safe, maybe add a FIXME?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1356
+int lambdaCaptures() {
+ int a = 0;
+ int b = 0;
----------------
It's pretty interesting that use-after-move fires for ints!
Someone might decide to "fix" that though, so probably best to use A like the
other tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119165/new/
https://reviews.llvm.org/D119165
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
