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

[Aaron Ballman via Phabricator via cfe-commits]

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: clang/include/clang/ASTMatchers/ASTMatchers.h:4629-4630
+///   matches `[x](){}`.
+AST_MATCHER_P(LambdaCapture, refersToVarDecl, internal::Matcher<VarDecl>,
+              InnerMatcher) {
+  auto *capturedVar = Node.getCapturedVar();
----------------
jcking1034 wrote:
> aaron.ballman wrote:
> > jcking1034 wrote:
> > > ymandel wrote:
> > > > aaron.ballman wrote:
> > > > > The name here is a bit unclear -- whether it is the matcher matching `int x;` or the `x` from the capture is not clear from the name. The comment suggests it's matching `x` from the capture, but I think it's actually matching the `int x;` variable declaration.
> > > > > 
> > > > > Being clear on what's matched here is important when we think about initializers:
> > > > > ```
> > > > > void foo() {
> > > > >   int x = 12;
> > > > >   auto f = [x = 100](){};
> > > > > }
> > > > > ```
> > > > > and
> > > > > ```
> > > > > lambdaExpr(hasAnyCapture(lambdaCapture(refersToVarDecl(hasName("x"), hasInitializer(integerLiteral(equals(100))))))
> > > > > ```
> > > > > Would you expect this to match? (This might be a good test case to add.)
> > > > In a similar vein, do we want a separate matcher on the name of the capture itself? e.g. an overload of `hasName`? And what about matchers for the initializers?  Those don't have to land in this patch, but do you think those would be doable?
> > > I would expect @aaron.ballman's initializer example to match, and I added a similar test case to the one  described. I think that if a capture does not have an initializer, then `refersToVarDecl` will match on the variable declaration before the lambda. However, if a capture does have an initializer, that initializer itself seems to be represented as a `VarDecl` in the AST, which is the `VarDecl` that gets matched.
> > > 
> > > For that reason, I think we may not need to have a separate matcher on the name of the capture itself. Additionally, since captures with/without initializers are both represented the same way, there may not be a good way to distinguish between them, so matchers for initializers may not be possible.
> > > I think that if a capture does not have an initializer, then refersToVarDecl will match on the variable declaration before the lambda. However, if a capture does have an initializer, that initializer itself seems to be represented as a VarDecl in the AST, which is the VarDecl that gets matched.
> > 
> > Oof, that'd be confusing! :-(
> > 
> > > For that reason, I think we may not need to have a separate matcher on the name of the capture itself.
> > 
> > Er, but there are init captures where you can introduce a whole new declaration. I think we do want to be able to match on that, right? e.g.,
> > ```
> > [x = 12](){ return x; }();
> > ```
> > 
> > > Additionally, since captures with/without initializers are both represented the same way, there may not be a good way to distinguish between them, so matchers for initializers may not be possible.
> > 
> > That's a bummer! :-( If this turns out to be a limitation, we should probably document it as such.
> For the example you've provided, these can be matched with the `refersToVarDecl` matcher, as seen in the test `LambdaCaptureTest_BindsToCaptureWithInitializer`. I've gone ahead and updated the documentation to include an example with an initializer.
> 
> Having that limitation with initializer representation is definitely a concern, though. Looking through the [[ https://clang.llvm.org/doxygen/LambdaCapture_8h_source.html | source ]] for the `LambdaCapture` class, the documentation for the `DeclAndBits` (line 42-48) suggests that there isn't a distinguishment between the two cases. However, do you think it's feasible to update the classes related to `LambdaCapture` obtain and store this information (possibly through updating the `LambdaCaptureKind` enum, updating the constructor/fields of the class, etc)?
> However, do you think it's feasible to update the classes related to LambdaCapture obtain and store this information (possibly through updating the LambdaCaptureKind enum, updating the constructor/fields of the class, etc)?

I think that would make sense (thought perhaps as an orthogonal patch). That we don't track init captures seems like a deficiency of the AST to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112491