[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Sam McCall via Phabricator via cfe-commits]

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