[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

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

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

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

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

Definitely reads better, updated the revision accordingly.

clang/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 (!getDerived().shouldVisitImplicitCode()) {
+    // Visit only the syntactic form if the clients are not interested in
+    // implicit compiler-generated semantic form.
+    TRY_TO(TraverseSynOrSemInitListExpr(
+        S->isSyntacticForm() ? S : S->getSyntacticForm(), Queue));
+    return true;
+  }
+
   TRY_TO(TraverseSynOrSemInitListExpr(
       S->isSemanticForm() ? S->getSyntacticForm() : S, Queue));
   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, {