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, capturesVar, internal::Matcher<VarDecl>,
----------------
update to new matcher name.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4650
+/// \endcode
+/// lambdaExpr(hasAnyCapture(lambdaCapture(refersToThis())))
+/// matches `[this]() { return cc; }`.
----------------
================
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:
> > 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.
Yeah, this seems orthogonal, if quite desirable. I imagine this will be a
frustration in the future. :( But, fixing the AST itself seems like quite a
lot of work.
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:4449
+
+TEST_P(ASTMatchersTest, RefersToThis) {
+ if (!GetParam().isCXX11OrLater()) {
----------------
rename?
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:2248
+
+TEST_P(ASTMatchersTest, LambdaCaptureTest_BindsToCaptureReferringToVarDecl) {
+ if (!GetParam().isCXX11OrLater()) {
----------------
and below as well.
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
