Visit lambda capture inits from RecursiveASTVisitor::TraverseLambdaCapture().

[mboehme]

Summary

rL277342 made RecursiveASTVisitor visit lambda capture initialization
expressions (these are the Exprs in LambdaExpr::capture_inits()).

jdennett identified two issues with rL277342 (see comments there for details):

• It visits initialization expressions for implicit lambda captures, even if shouldVisitImplicitCode() returns false.

• It visits initialization expressions for init captures twice (because these were already traveresed in TraverseLambdaCapture() before rL277342)

This patch fixes these issues and moves the code for traversing initialization
expressions into TraverseLambdaCapture().

This patch also makes two changes required for the tests:

• It adds Lang_CXX14 to the Language enum in TestVisitor.

• It adds a parameter to ExpectedLocationVisitor::ExpectMatch() that specifies the number of times a match is expected to be seen.

[klimek]

include/clang/AST/RecursiveASTVisitor.h

892 ↗

(On Diff #66942)

I'd rather pass in the offset than doing math with what's semantically a pointer vs an iterator.

unittests/Tooling/RecursiveASTVisitorTestExprVisitor.cpp

236 ↗

(On Diff #66942)

I'd remove the extra scope.

[mboehme]

Responses to reviewer comments

In particular, changed the interface of TraverseLambdaCapture

[mboehme]

Update to a clean diff.

(Sorry, the last diff included a large number of extraneous changes by others.)

[mboehme]

include/clang/AST/RecursiveASTVisitor.h

895 ↗

(On Diff #68139)

I've now decided to do the interface change for TraverseLambdaCapture in this same patch.

There are a few users of TraverseLambdaCapture in clang-tools-extra; the required changes are in D23543. The intent is to wait until the reviews for both patches are complete, then submit them at the same time.

892 ↗

(On Diff #66942)

I think the cleanest way to do this would be to pass in both the LambdaCapture and the Expr for the corresponding initialization (and handle the offsets in TraverseLambdaExpr).

This will first require transitioning all users of TraverseLambdaCapture to the new interface, which I'll do in a separate patch.

[alexfh]

LGTM with a couple of nits.

unittests/Tooling/RecursiveASTVisitorTestExprVisitor.cpp

221 ↗

(On Diff #68139)

nit: Should this be two different tests for simplicity?

228 ↗

(On Diff #68139)

nit: Remove all the spaces in /*Times=*/2.

239 ↗

(On Diff #68139)

So, C++14 allows you do that? Wow, TIL something new about C++ ;)

[mboehme]

Changes in response to reviewer comments

[mboehme]

unittests/Tooling/RecursiveASTVisitorTestExprVisitor.cpp

239 ↗

(On Diff #68139)

;)

cfe/trunk/include/clang/AST/RecursiveASTVisitor.h

public:
   /// \brief Recursively visit a constructor initializer.  This
   /// automatically dispatches to another visitor for the initializer
   /// expression, but not for the name of the initializer, so may
   /// be overridden for clients that need access to the name.
   ///
   /// \returns false if the visitation was terminated early, true otherwise.
   bool TraverseConstructorInitializer(CXXCtorInitializer *Init);
 
-  /// \brief Recursively visit a lambda capture.
+  /// \brief Recursively visit a lambda capture. \c Init is the expression that
+  /// will be used to initialize the capture.
   ///
   /// \returns false if the visitation was terminated early, true otherwise.
-  bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C);
+  bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C,
+                             Expr *Init);
 
   /// \brief Recursively visit the body of a lambda expression.
   ///
   /// This provides a hook for visitors that need more context when visiting
   /// \c LE->getBody().
   ///
   /// \returns false if the visitation was terminated early, true otherwise.
   bool TraverseLambdaBody(LambdaExpr *LE, DataRecursionQueue *Queue = nullptr);
if (getDerived().shouldVisitImplicitCode())
     }
 
   return true;
 }
 
 template <typename Derived>
 bool
 RecursiveASTVisitor<Derived>::TraverseLambdaCapture(LambdaExpr *LE,
-                                                    const LambdaCapture *C) {
+                                                    const LambdaCapture *C,
+                                                    Expr *Init) {
   if (LE->isInitCapture(C))
     TRY_TO(TraverseDecl(C->getCapturedVar()));
+  else
+    TRY_TO(TraverseStmt(Init));
   return true;
 }
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseLambdaBody(
     LambdaExpr *LE, DataRecursionQueue *Queue) {
   TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(LE->getBody());
   return true;

 
 DEF_TRAVERSE_STMT(CXXTemporaryObjectExpr, {
   // This is called for code like 'return T()' where T is a class type.
   TRY_TO(TraverseTypeLoc(S->getTypeSourceInfo()->getTypeLoc()));
 })
 
 // Walk only the visible parts of lambda expressions.
 DEF_TRAVERSE_STMT(LambdaExpr, {
-  for (LambdaExpr::capture_iterator C = S->explicit_capture_begin(),
-                                    CEnd = S->explicit_capture_end();
-       C != CEnd; ++C) {
-    TRY_TO(TraverseLambdaCapture(S, C));
+  for (unsigned I = 0, N = S->capture_size(); I != N; ++I) {
+    const LambdaCapture *C = S->capture_begin() + I;
+    if (C->isExplicit() || getDerived().shouldVisitImplicitCode()) {
+      TRY_TO(TraverseLambdaCapture(S, C, S->capture_init_begin()[I]));
     }
-  for (Expr *Init : S->capture_inits()) {
-    TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(Init);
   }
 
   TypeLoc TL = S->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
   FunctionProtoTypeLoc Proto = TL.castAs<FunctionProtoTypeLoc>();
 
   if (S->hasExplicitParameters() && S->hasExplicitResultType()) {
     // Visit the whole type.
     TRY_TO(TraverseTypeLoc(TL));

cfe/trunk/lib/Index/IndexBody.cpp

for (DeclGroupRef::iterator I = DG.begin(), E = DG.end(); I != E; ++I) {
         continue;
       if (!IndexCtx.isFunctionLocalDecl(D))
         IndexCtx.indexTopLevelDecl(D);
     }
 
     return true;
   }
 
-  bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C) {
+  bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C,
+                             Expr *Init) {
     if (C->capturesThis() || C->capturesVLAType())
       return true;
 
     if (C->capturesVariable() && IndexCtx.shouldIndexFunctionLocalSymbols())
       return IndexCtx.handleReference(C->getCapturedVar(), C->getLocation(),
                                       Parent, ParentDC, SymbolRoleSet());
 
     // FIXME: Lambda init-captures.

cfe/trunk/unittests/Tooling/RecursiveASTVisitorTestExprVisitor.cpp

TEST(RecursiveASTVisitor, CanSkipImplicitMemberInitializations) {
   EXPECT_TRUE(Visitor.runOver(
       "struct WithCtor { WithCtor(); }; \n"
       "struct Simple { WithCtor w; }; \n"
       "int main() { Simple s; }\n"));
 }
 
 class DeclRefExprVisitor : public ExpectedLocationVisitor<DeclRefExprVisitor> {
 public:
+  DeclRefExprVisitor() : ShouldVisitImplicitCode(false) {}
+
+  bool shouldVisitImplicitCode() const { return ShouldVisitImplicitCode; }
+
+  void setShouldVisitImplicitCode(bool NewValue) {
+    ShouldVisitImplicitCode = NewValue;
+  }
+
   bool VisitDeclRefExpr(DeclRefExpr *Reference) {
     Match(Reference->getNameInfo().getAsString(), Reference->getLocation());
     return true;
   }
+
+private:
+  bool ShouldVisitImplicitCode;
 };
 
 TEST(RecursiveASTVisitor, VisitsBaseClassTemplateArguments) {
   DeclRefExprVisitor Visitor;
   Visitor.ExpectMatch("x", 2, 3);
   EXPECT_TRUE(Visitor.runOver(
     "void x(); template <void (*T)()> class X {};\nX<x> y;"));
 }

 
 TEST(RecursiveASTVisitor, VisitsCallExpr) {
   DeclRefExprVisitor Visitor;
   Visitor.ExpectMatch("x", 1, 22);
   EXPECT_TRUE(Visitor.runOver(
     "void x(); void y() { x(); }"));
 }
 
-TEST(RecursiveASTVisitor, VisitsLambdaCaptureInit) {
+TEST(RecursiveASTVisitor, VisitsExplicitLambdaCaptureInit) {
   DeclRefExprVisitor Visitor;
   Visitor.ExpectMatch("i", 1, 20);
   EXPECT_TRUE(Visitor.runOver(
-    "void f() { int i; [i]{}; };",
+    "void f() { int i; [i]{}; }",
     DeclRefExprVisitor::Lang_CXX11));
 }
 
+TEST(RecursiveASTVisitor, VisitsUseOfImplicitLambdaCapture) {
+  DeclRefExprVisitor Visitor;
+  Visitor.ExpectMatch("i", 1, 24);
+  EXPECT_TRUE(Visitor.runOver(
+    "void f() { int i; [=]{ i; }; }",
+    DeclRefExprVisitor::Lang_CXX11));
+}
+
+TEST(RecursiveASTVisitor, VisitsImplicitLambdaCaptureInit) {
+  DeclRefExprVisitor Visitor;
+  Visitor.setShouldVisitImplicitCode(true);
+  // We're expecting the "i" in the lambda to be visited twice:
+  // - Once for the DeclRefExpr in the lambda capture initialization (whose
+  //   source code location is set to the first use of the variable).
+  // - Once for the DeclRefExpr for the use of "i" inside the lambda.
+  Visitor.ExpectMatch("i", 1, 24, /*Times=*/2);
+  EXPECT_TRUE(Visitor.runOver(
+    "void f() { int i; [=]{ i; }; }",
+    DeclRefExprVisitor::Lang_CXX11));
+}
+
+TEST(RecursiveASTVisitor, VisitsLambdaInitCaptureInit) {
+  DeclRefExprVisitor Visitor;
+  Visitor.ExpectMatch("i", 1, 24);
+  EXPECT_TRUE(Visitor.runOver(
+    "void f() { int i; [a = i + 1]{}; }",
+    DeclRefExprVisitor::Lang_CXX14));
+}
+
 /* FIXME: According to Richard Smith this is a bug in the AST.
 TEST(RecursiveASTVisitor, VisitsBaseClassTemplateArgumentsInInstantiation) {
   DeclRefExprVisitor Visitor;
   Visitor.ExpectMatch("x", 3, 43);
   EXPECT_TRUE(Visitor.runOver(
     "template <typename T> void x();\n"
     "template <void (*T)()> class X {};\n"
     "template <typename T> class Y : public X< x<T> > {};\n"

cfe/trunk/unittests/Tooling/TestVisitor.h

public:
   TestVisitor() { }
 
   virtual ~TestVisitor() { }
 
   enum Language {
     Lang_C,
     Lang_CXX98,
     Lang_CXX11,
+    Lang_CXX14,
     Lang_OBJC,
     Lang_OBJCXX11,
     Lang_CXX = Lang_CXX98
   };
 
   /// \brief Runs the current AST visitor over the given code.
   bool runOver(StringRef Code, Language L = Lang_CXX) {
     std::vector<std::string> Args;
     switch (L) {
       case Lang_C: Args.push_back("-std=c99"); break;
       case Lang_CXX98: Args.push_back("-std=c++98"); break;
       case Lang_CXX11: Args.push_back("-std=c++11"); break;
+      case Lang_CXX14: Args.push_back("-std=c++14"); break;
       case Lang_OBJC: Args.push_back("-ObjC"); break;
       case Lang_OBJCXX11:
         Args.push_back("-ObjC++");
         Args.push_back("-std=c++11");
         Args.push_back("-fblocks");
         break;
     }
     return tooling::runToolOnCodeWithArgs(CreateTestAction(), Code, Args);
public:
   /// Any number of matches can be disallowed.
   void DisallowMatch(Twine Match, unsigned Line, unsigned Column) {
     DisallowedMatches.push_back(MatchCandidate(Match, Line, Column));
   }
 
   /// \brief Expect 'Match' to occur at the given 'Line' and 'Column'.
   ///
   /// Any number of expected matches can be set by calling this repeatedly.
-  /// Each is expected to be matched exactly once.
-  void ExpectMatch(Twine Match, unsigned Line, unsigned Column) {
-    ExpectedMatches.push_back(ExpectedMatch(Match, Line, Column));
+  /// Each is expected to be matched 'Times' number of times. (This is useful in
+  /// cases in which different AST nodes can match at the same source code
+  /// location.)
+  void ExpectMatch(Twine Match, unsigned Line, unsigned Column,
+                   unsigned Times = 1) {
+    ExpectedMatches.push_back(ExpectedMatch(Match, Line, Column, Times));
   }
 
   /// \brief Checks that all expected matches have been found.
   ~ExpectedLocationVisitor() override {
     for (typename std::vector<ExpectedMatch>::const_iterator
              It = ExpectedMatches.begin(), End = ExpectedMatches.end();
          It != End; ++It) {
       It->ExpectFound();
struct MatchCandidate {
     friend std::ostream &operator<<(std::ostream &Stream,
                                     MatchCandidate const &Match) {
       return Stream << Match.ExpectedName
                     << " at " << Match.LineNumber << ":" << Match.ColumnNumber;
     }
   };
 
   struct ExpectedMatch {
-    ExpectedMatch(Twine Name, unsigned LineNumber, unsigned ColumnNumber)
-      : Candidate(Name, LineNumber, ColumnNumber), Found(false) {}
+    ExpectedMatch(Twine Name, unsigned LineNumber, unsigned ColumnNumber,
+                  unsigned Times)
+        : Candidate(Name, LineNumber, ColumnNumber), TimesExpected(Times),
+          TimesSeen(0) {}
 
     void UpdateFor(StringRef Name, FullSourceLoc Location, SourceManager &SM) {
       if (Candidate.Matches(Name, Location)) {
-        EXPECT_TRUE(!Found);
-        Found = true;
-      } else if (!Found && Candidate.PartiallyMatches(Name, Location)) {
+        EXPECT_LT(TimesSeen, TimesExpected);
+        ++TimesSeen;
+      } else if (TimesSeen < TimesExpected &&
+                 Candidate.PartiallyMatches(Name, Location)) {
         llvm::raw_string_ostream Stream(PartialMatches);
         Stream << ", partial match: \"" << Name << "\" at ";
         Location.print(Stream, SM);
       }
     }
 
     void ExpectFound() const {
-      EXPECT_TRUE(Found)
+      EXPECT_EQ(TimesExpected, TimesSeen)
           << "Expected \"" << Candidate.ExpectedName
           << "\" at " << Candidate.LineNumber
           << ":" << Candidate.ColumnNumber << PartialMatches;
     }
 
     MatchCandidate Candidate;
     std::string PartialMatches;
-    bool Found;
+    unsigned TimesExpected;
+    unsigned TimesSeen;
   };
 
   std::vector<MatchCandidate> DisallowedMatches;
   std::vector<ExpectedMatch> ExpectedMatches;
 };
 }
 
 #endif