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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
