[PATCH] D112491: Add `LambdaCapture`-related matchers.

This revision was automatically updated to reflect the committed changes. Closed by commit rGc0b298fc213c: Add `LambdaCapture`-related matchers. (authored by jcking1034, committed by ymandel). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112491: Add `LambdaCapture`-related matchers.

aaron.ballman accepted this revision. aaron.ballman added a comment. This continues to LGTM, thank you for it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 added a comment. @aaron.ballman Just wanted to confirm with you that the work here and release notes look good and can be wrapped up so that I can have @ymandel submit! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 marked an inline comment as done. jcking1034 added a comment. @fowles @aaron.ballman I'll take a look at `forEachCapture` in the next patch. Also, I've discovered that the `VarDecl` node has a member function `isInitCapture` that seems like it could allow us to distinguish between

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 marked an inline comment as done. jcking1034 added inline comments. Comment at: clang/docs/ReleaseNotes.rst:223-229 +- ``LambdaCapture`` AST Matchers are now available. These matchers allow for + the binding of ``LambdaCapture`` nodes, and include the

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 384835. jcking1034 added a comment. Update release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files: clang/docs/LibASTMatchersReference.html

[PATCH] D112491: Add `LambdaCapture`-related matchers.

fowles added a comment. In D112491#3108631 , @aaron.ballman wrote: > That might be good follow-on work (I wouldn't insist on it for this patch > though). Completely agreed, just something that occurred to me as the next thing I will need when

[PATCH] D112491: Add `LambdaCapture`-related matchers.

aaron.ballman added a comment. In D112491#3107361 , @fowles wrote: > Do we also want a `forEachCapture` matcher? That might be good follow-on work (I wouldn't insist on it for this patch though). Comment at:

[PATCH] D112491: Add `LambdaCapture`-related matchers.

fowles added a comment. Do we also want a `forEachCapture` matcher? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 ___ cfe-commits mailing list

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 384578. jcking1034 added a comment. Rebase onto main. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files: clang/docs/LibASTMatchersReference.html

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 384114. jcking1034 added a comment. Update documentation for `capturesVar` matcher. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files:

[PATCH] D112491: Add `LambdaCapture`-related matchers.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for the changes, this looks great to me now. Comment at: clang/docs/LibASTMatchersReference.html:8368

[PATCH] D112491: Add `LambdaCapture`-related matchers.

fowles added a comment. This is great! Comment at: clang/docs/LibASTMatchersReference.html:8368 +lambdaExpr(hasAnyCapture(lambdaCapture(capturesVar(hasName("x", +capturesVar(hasName("x")) matches `int x` and `x = 1`. I think this should be "matches `x`

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 383823. jcking1034 marked 4 inline comments as done. jcking1034 added a comment. Update missed names; remove original implementations of `hasAnyCapture`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112491: Add `LambdaCapture`-related matchers.

ymandel added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4630-4632 +/// lambdaExpr(hasAnyCapture(lambdaCapture(refersToVarDecl(hasName("x", +/// refersToVarDecl(hasName("x")) matches `int x` and `x = 1`. +AST_MATCHER_P(LambdaCapture,

[PATCH] D112491: Add `LambdaCapture`-related matchers.

ymandel added a comment. +1 to removing the old versions of these matchers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 ___ cfe-commits mailing list

[PATCH] D112491: Add `LambdaCapture`-related matchers.

aaron.ballman added a comment. In D112491#3096935 , @sammccall wrote: > This looks really sensible to me too. Some naming bikeshedding, but my main > question is migration. > > Supporting two things is indeed annoying and confusing. I agree it's not >

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4652 +/// matches `[this]() { return cc; }`. +AST_MATCHER(LambdaCapture, refersToThis) { return Node.capturesThis(); } + sammccall wrote: > Again, why `refersToThis`

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 383458. jcking1034 marked 2 inline comments as done. jcking1034 added a comment. Update matcher names for clarity. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491

[PATCH] D112491: Add `LambdaCapture`-related matchers.

sammccall added a comment. This looks really sensible to me too. Some naming bikeshedding, but my main question is migration. Supporting two things is indeed annoying and confusing. I agree it's not worth keeping the old way forever just to avoid writing `refersToVarDecl`. The question is: how

[PATCH] D112491: Add `LambdaCapture`-related matchers.

aaron.ballman added a reviewer: sammccall. aaron.ballman added a subscriber: sammccall. aaron.ballman added a comment. In general, I'm happy with this. Adding @sammccall in case I've missed anything regarding the AST matcher internals or design concerns. Comment at:

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 383128. jcking1034 added a comment. Regenerate documentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files: clang/docs/LibASTMatchersReference.html

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 383126. jcking1034 added a comment. Deprecate original overloads of `hasAnyCapture`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files:

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 marked an inline comment as not done. jcking1034 added a comment. I agree that having two ways to match the same thing is a usability concern and could definitely be confusing. Deprecating non-bindable matchers could be a possibility and is probably the right way to go if we choose

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 383115. jcking1034 added a comment. Update comment with additional example. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files:

[PATCH] D112491: Add `LambdaCapture`-related matchers.

aaron.ballman added a comment. In D112491#3088363 , @jcking1034 wrote: > @aaron.ballman for the purpose of these matchers, what @ymandel said is > correct - the goal is to allow `LambdaCapture`s themselves to be bindable. Should we be discussing

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 added a comment. @aaron.ballman for the purpose of these matchers, what @ymandel said is correct - the goal is to allow `LambdaCapture`s themselves to be bindable. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4629-4630 +/// matches `[x](){}`.

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 382410. jcking1034 added a comment. Update comments and tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491 Files: clang/docs/LibASTMatchersReference.html

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 382397. jcking1034 marked an inline comment as done. jcking1034 added a comment. Update comment with example Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 updated this revision to Diff 382391. jcking1034 marked 3 inline comments as done. jcking1034 added a comment. Revert changes to ASTMatchFinder Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112491/new/ https://reviews.llvm.org/D112491

[PATCH] D112491: Add `LambdaCapture`-related matchers.

ymandel added a comment. Nice! Per Aaron's comment, it's probably worth expanding the patch description to explain the motivation. I assume that the key is making these first-class and therefore bindable? Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:170

[PATCH] D112491: Add `LambdaCapture`-related matchers.

aaron.ballman added a comment. I'm a bit confused as to why this is necessary. We already have `lambdaExpr(hasAnyCapture(varDecl(hasName("whatever"` and `lambdaExpr(hasAnyCapture(cxxThisExpr()))`, so I'm not certain why we need this as a parallel construct? Comment at:

[PATCH] D112491: Add `LambdaCapture`-related matchers.

jcking1034 created this revision. jcking1034 added reviewers: ymandel, aaron.ballman. jcking1034 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This provides better support for `LambdaCapture`s, and implements several