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 to include these
matchers, but I'm not sure of if doing so will have any side effects.
================
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();
----------------
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)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112491/new/
https://reviews.llvm.org/D112491
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
