r228144 - Rewrite r228138 to be somewhat saner.

[Richard Smith]

On Sun, Feb 22, 2015 at 1:13 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com>
wrote:

> 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 ?


I think the right thing to do here is for initialization to update the
syntactic form of the InitListExpr to include implicit conversions, and
then for RecursiveASTVisitor to only visit the syntactic form (at least by
default). The problem we have right now is that both the syntactic and
semantic forms are lossy (and in fact there are cases where *neither* of
them captures certain information), so picking just one of them to visit
will fail to visit some expressions. We can't make the semantic form carry
the complete information, but we can do so for the syntactic form.

Example of why our current AST representation is broken:

struct S { void (*p)(int); };
template<typename T> void f(T);
void g(int);
S s = { f, .p = g }; // neither syntactic nor semantic form contains a
reference to f<int>.

> 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
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150222/9f4afa2c/attachment.html>