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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:403
                hasArgument(0, declRefExpr().bind("arg")),
-               anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
-                     hasAncestor(functionDecl().bind("containing-func"))),
----------------
I believe this can be done more simply.

IIUC, the root of the problem is that a std::move() in a lambda capture is being associated with the lambda, when in fact it should be associated with the function that contains the lambda.

This is because the `hasAncestor(lambdaExpr())` matches not just a `std::move` inside the body of the lambda, but also in captures.

I believe this can be solved simply by changing this line so that it only matches a `std::move` inside the body of the lambda, i.e. something like this:

```
hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda"))))
```

This would then no longer match a `std::move` in a capture; the existing logic would instead associate it with the function that contains the lambda.
```


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:418
+      forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+          hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))))))),
+      AncestorMatcher);
----------------
sammccall wrote:
> 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?
If this isn't a deliberate restriction, can you add a test for this?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1355
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;
----------------
Can  you put this test with the other tests for lambdas? My suggestion would be to insert it below line 418.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1356
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;
----------------
ivanmurashko wrote:
> sammccall wrote:
> > 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.
> There is also another case that I want to address as a separate patch.
> ```
> void autoCapture() {
>   auto var = [](auto &&res) {
>     auto f = [blk = std::move(res)]() {};
>     return std::move(res);
>   };
> }
> ```
> This one is matched as `UnresolvedLookupExpr` and requires another modified matcher
I assume the reason you don't want to address this case within this particular patch is that it requires a lot of additional code?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1363
+    // CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+    // CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+    return aa + bb + cc;
----------------
Can you put these `CHECK-NOTES` after the line `return a + b + c`?

The other tests put the `CHECK-NOTES` after the use, not the move, and it would be nice to be consistent with that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119165/new/

https://reviews.llvm.org/D119165