Divide TraverseInitListExpr to a different function for each form.

[angelgarcia]

Summary

create TraverseSyntacticInitListExpr and TraverseSemanticInitListExpr.

[klimek]

include/clang/AST/RecursiveASTVisitor.h

2083

Did you try putting an assert(S->isSemanticForm()); (or the reverse) here?

[angelgarcia]

Yes, it breaks a few tests:

FAIL: Clang :: Analysis/operator-calls.cpp (598 of 8596)

FAIL: Clang :: Analysis/misc-ps-region-store.cpp (599 of 8596)

FAIL: Clang :: Analysis/array-struct-region.c (602 of 8596)

[klimek]

I think it's worth figuring out when this is called with the semantic or
syntactic version and why this can't lead to double visitation. Then add a
comment while you're changing the method so the next person doesn't have to
figure it all out :)

[angelgarcia]

Add some comments.

[klimek]

include/clang/AST/RecursiveASTVisitor.h

2065–2075

Ok, I'd now pull out the first line of these functions into the TraverseInitListExpr, and then we only need one function here, right?

2077–2081

I think we can remove this - I'm not sure it adds value.

[klimek]

+richard to tell us whether that comment is correct :)

[angelgarcia]

Use only one function.

I find somewhat frustating that getSemanticForm() returns nullptr if the object is already in its semantic form (and the same for isSyntacticForm()). Something like QualType::getNonReferenceType(), that returns the object itself if it is already a non-reference type, would be more intuitive.

[klimek]

LG

include/clang/AST/RecursiveASTVisitor.h

public:
   /// \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);
 
+  /// \brief Recursively visit the syntactic or semantic form of an
+  /// initialization list.
+  ///
+  /// \returns false if the visitation was terminated early, true otherwise.
+  bool TraverseSynOrSemInitListExpr(InitListExpr *S);
+
   // ---- Methods on Attrs ----
 
   // \brief Visit an attribute.
   bool VisitAttr(Attr *A) { return true; }
 
 // Declare Traverse* and empty Visit* for all Attr classes.
 #define ATTR_VISITOR_DECLS_ONLY
 #include "clang/AST/AttrVisitor.inc"

 DEF_TRAVERSE_STMT(CXXReinterpretCastExpr, {
   TRY_TO(TraverseTypeLoc(S->getTypeInfoAsWritten()->getTypeLoc()));
 })
 
 DEF_TRAVERSE_STMT(CXXStaticCastExpr, {
   TRY_TO(TraverseTypeLoc(S->getTypeInfoAsWritten()->getTypeLoc()));
 })
 
-// InitListExpr is a tricky one, because we want to do all our work on
-// the syntactic form of the listexpr, but this method takes the
-// semantic form by default.  We can't use the macro helper because it
-// calls WalkUp*() on the semantic form, before our code can convert
-// to the syntactic form.
 template <typename Derived>
-bool RecursiveASTVisitor<Derived>::TraverseInitListExpr(InitListExpr *S) {
-  InitListExpr *Syn = S->isSemanticForm() ? S->getSyntacticForm() : S;
-  if (Syn) {
-    TRY_TO(WalkUpFromInitListExpr(Syn));
+bool RecursiveASTVisitor<Derived>::TraverseSynOrSemInitListExpr(InitListExpr *S) {
+  if (S) {
+    TRY_TO(WalkUpFromInitListExpr(S));
     // All we need are the default actions.  FIXME: use a helper function.
-    for (Stmt *SubStmt : Syn->children()) {
+    for (Stmt *SubStmt : S->children()) {
       TRY_TO(TraverseStmt(SubStmt));
     }
   }
-  InitListExpr *Sem = S->isSemanticForm() ? S : S->getSemanticForm();
-  if (Sem) {
-    TRY_TO(WalkUpFromInitListExpr(Sem));
-    for (Stmt *SubStmt : Sem->children()) {
-      TRY_TO(TraverseStmt(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.
+//
+// There is no guarantee about which form \p S takes when this method is called.
+template <typename Derived>
+bool RecursiveASTVisitor<Derived>::TraverseInitListExpr(InitListExpr *S) {
+  TRY_TO(TraverseSynOrSemInitListExpr(
+      S->isSemanticForm() ? S->getSyntacticForm() : S));
+  TRY_TO(TraverseSynOrSemInitListExpr(
+      S->isSemanticForm() ? S : S->getSemanticForm()));
   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).
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseGenericSelectionExpr(