r228144 - Rewrite r228138 to be somewhat saner.

[Argyrios Kyrtzidis]

Hi Daniel,

Your change to always visit both syntactic and semantic forms of InitListExprs, by default, has downsides:

- It may be surprising and undesirable for some RecursiveASTVisitor clients that they see the same nodes visited twice
- It can trigger exponential behavior, significantly degrading performance with huge initializer exprs; for example try visiting "llvm/src/lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp” (and look into what 'lib/Target/X86/X86GenDisassemblerTables.inc’ contains).

How about we make TraverseInitListExpr(InitListExpr *S) visit the semantic form only by default and add something like “visitationForInitLists()” that returns an enum for semantic/syntactic/both allowing RecursiveASTVisitor clients to control this ?

> On Feb 4, 2015, at 6:29 AM, Daniel Jasper <djasper at google.com> wrote:
> 
> Author: djasper
> Date: Wed Feb  4 08:29:47 2015
> New Revision: 228144
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=228144&view=rev
> Log:
> Rewrite r228138 to be somewhat saner.
> 
> While probably technically correct, the solution r228138 was quite hard
> to read/understand. This should be simpler.
> 
> Also added a test to ensure that we are still visiting the syntactic form
> as well.
> 
> Modified:
>    cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
>    cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
> 
> Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=228144&r1=228143&r2=228144&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)
> +++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h Wed Feb  4 08:29:47 2015
> @@ -2032,20 +2032,21 @@ DEF_TRAVERSE_STMT(CXXStaticCastExpr, {
> // to the syntactic form.
> template <typename Derived>
> bool RecursiveASTVisitor<Derived>::TraverseInitListExpr(InitListExpr *S) {
> -  if (InitListExpr *Syn = S->getSyntacticForm())
> -    S = Syn;
> -  TRY_TO(WalkUpFromInitListExpr(S));
> -  // All we need are the default actions.  FIXME: use a helper function.
> -  for (Stmt::child_range range = S->children(); range; ++range) {
> -    TRY_TO(TraverseStmt(*range));
> -  }
> -  if (InitListExpr *Syn = S->getSemanticForm()) {
> +  InitListExpr *Syn = S->isSemanticForm() ? S->getSyntacticForm() : S;
> +  if (Syn) {
>     TRY_TO(WalkUpFromInitListExpr(Syn));
>     // All we need are the default actions.  FIXME: use a helper function.
>     for (Stmt::child_range range = Syn->children(); range; ++range) {
>       TRY_TO(TraverseStmt(*range));
>     }
>   }
> +  InitListExpr *Sem = S->isSemanticForm() ? S : S->getSemanticForm();
> +  if (Sem) {
> +    TRY_TO(WalkUpFromInitListExpr(Sem));
> +    for (Stmt::child_range range = Sem->children(); range; ++range) {
> +      TRY_TO(TraverseStmt(*range));
> +    }
> +  }
>   return true;
> }
> 
> 
> Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp?rev=228144&r1=228143&r2=228144&view=diff
> ==============================================================================
> --- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp (original)
> +++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp Wed Feb  4 08:29:47 2015
> @@ -3148,6 +3148,8 @@ TEST(InitListExpression, MatchesInitList
>                       "void f();"
>                       "S s[1] = { &f };",
>                       declRefExpr(to(functionDecl(hasName("f"))))));
> +  EXPECT_TRUE(
> +      matches("int i[1] = {42, [0] = 43};", integerLiteral(equals(42))));
> }
> 
> TEST(UsingDeclaration, MatchesUsingDeclarations) {
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits