[AST] Treat semantic form of InitListExpr as implicit code in traversals

[ilya-biryukov]

Summary

In particular, do not traverse the semantic form if shouldVisitImplicitCode()
returns false.

This simplifies the common case of traversals, avoiding the need to
worry about some expressions being traversed twice.

No tests break after the change, the change would allow to simplify at
least one of the usages, i.e. r366070 which had to handle this in
clangd.

[gribozavr]

Please add tests to llvm/tools/clang/unittests/Tooling/RecursiveASTVisitorTests/.

clang/include/clang/AST/RecursiveASTVisitor.h

2332 ↗

(On Diff #209913)

Instead of adding a whole new if statement, could you wrap the second existing TRY_TO in if(shouldVisitImplicitCode()) ?

[ilya-biryukov]

clang/include/clang/AST/RecursiveASTVisitor.h

2332 ↗

(On Diff #209913)

Despite looking very similar, that would not be equivalent to the current version.

For most init lists (that do not have alternative "form"), the following invariants hold:

InitList* E = ...;
assert(E->isSemanticForm());
assert(E->isSyntacticForm()); 
assert(E->getSynacticForm() == nullptr);

This subtle fact means the current code does not traversed the list twice if they do not have an alternative form (either semantic or syntactic).

Now, if we only run the first statement, we will call TraverseSynOrSemInitListExpr(S->getSyntacticForm()) and S->getSyntacticForm() returns null. So we don't traverse anything.

I tried various ways to keep both calls, but they all ended up being too complicated, hence the final version. Let me know if you see a better way to address this.

[ilya-biryukov]

• Add a test.

[ilya-biryukov]

clang/include/clang/AST/RecursiveASTVisitor.h

2332 ↗

(On Diff #209913)

To make the last sentence less confusing:
I tried various ways to keep only two calls, but they were too complicated and I ended up introducing an extra call to TraverseSyn... instead.

[gribozavr]

clang/include/clang/AST/RecursiveASTVisitor.h

2332 ↗

(On Diff #209913)

assert(E->getSynacticForm() == nullptr);

That's... a really nice API.

What do you think about the following:

if (S->isSyntacticForm() && S->isSemanticForm()) {
  // `S` does not have alternative forms, traverse the only form that's available.
  TRY_TO(TraverseSynOrSemInitListExpr(S, Queue));
  return true;
}

TRY_TO(TraverseSynOrSemInitListExpr(
  S->isSemanticForm() ? S->getSyntacticForm() : S, Queue));
if (getDerived().shouldVisitImplicitCode()) {
  TRY_TO(TraverseSynOrSemInitListExpr(
    S->isSyntacticForm() ? S->getSemanticForm() : S, Queue));
}

[ilya-biryukov]

• Rewrite code as suggested in the review

clang/include/clang/AST/RecursiveASTVisitor.h

2332 ↗

(On Diff #209913)

Definitely reads better, updated the revision accordingly.

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

if (S) {
     // All we need are the default actions.  FIXME: use a helper function.
     for (Stmt *SubStmt : S->children()) {
       TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(SubStmt);
     }
   }
   return true;
 }
 
-// This method is called once for each pair of syntactic and semantic
-// InitListExpr, and it traverses the subtrees defined by the two forms. This
-// may cause some of the children to be visited twice, if they appear both in
-// the syntactic and the semantic form.
+// If shouldVisitImplicitCode() returns false, this method traverses only the
+// syntactic form of InitListExpr.
+// If shouldVisitImplicitCode() return true, this method is called once for
+// each pair of syntactic and semantic InitListExpr, and it traverses the
+// subtrees defined by the two forms. This may cause some of the children to be
+// visited twice, if they appear both in the syntactic and the semantic form.
 //
 // There is no guarantee about which form \p S takes when this method is called.
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseInitListExpr(
     InitListExpr *S, DataRecursionQueue *Queue) {
+  if (S->isSemanticForm() && S->isSyntacticForm()) {
+    // `S` does not have alternative forms, traverse only once.
+    TRY_TO(TraverseSynOrSemInitListExpr(S, Queue));
+    return true;
+  }
   TRY_TO(TraverseSynOrSemInitListExpr(
       S->isSemanticForm() ? S->getSyntacticForm() : S, Queue));
+  if (getDerived().shouldVisitImplicitCode()) {
+    // Only visit the semantic form if the clients are interested in implicit
+    // compiler-generated.
     TRY_TO(TraverseSynOrSemInitListExpr(
         S->isSemanticForm() ? S : S->getSemanticForm(), Queue));
+  }
   return true;
 }
 
 // GenericSelectionExpr is a special case because the types and expressions
 // are interleaved.  We also need to watch out for null types (default
 // generic associations).
 DEF_TRAVERSE_STMT(GenericSelectionExpr, {
   TRY_TO(TraverseStmt(S->getControllingExpr()));

cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/InitListExprPreOrder.cpp

 
 namespace {
 
 // Check to ensure that InitListExpr is visited twice, once each for the
 // syntactic and semantic form.
 class InitListExprPreOrderVisitor
     : public ExpectedLocationVisitor<InitListExprPreOrderVisitor> {
 public:
+  InitListExprPreOrderVisitor(bool VisitImplicitCode)
+      : VisitImplicitCode(VisitImplicitCode) {}
+
+  bool shouldVisitImplicitCode() const { return VisitImplicitCode; }
+
   bool VisitInitListExpr(InitListExpr *ILE) {
     Match(ILE->isSemanticForm() ? "semantic" : "syntactic", ILE->getBeginLoc());
     return true;
   }
+
+private:
+  bool VisitImplicitCode;
 };
 
 TEST(RecursiveASTVisitor, InitListExprIsPreOrderVisitedTwice) {
-  InitListExprPreOrderVisitor Visitor;
+  InitListExprPreOrderVisitor Visitor(/*VisitImplicitCode=*/true);
   Visitor.ExpectMatch("syntactic", 2, 21);
   Visitor.ExpectMatch("semantic", 2, 21);
   EXPECT_TRUE(Visitor.runOver("struct S { int x; };\n"
                               "static struct S s = {.x = 0};\n",
                               InitListExprPreOrderVisitor::Lang_C));
 }
 
+TEST(RecursiveASTVisitor, InitListExprVisitedOnceWhenNoImplicit) {
+  InitListExprPreOrderVisitor Visitor(/*VisitImplicitCode=*/false);
+  Visitor.ExpectMatch("syntactic", 2, 21);
+  Visitor.DisallowMatch("semantic", 2, 21);
+  EXPECT_TRUE(Visitor.runOver("struct S { int x; };\n"
+                              "static struct S s = {.x = 0};\n",
+                              InitListExprPreOrderVisitor::Lang_C));
+}
+
 } // end anonymous namespace