<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Feb 22, 2015 at 1:13 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Daniel,<br>
<br>
Your change to always visit both syntactic and semantic forms of InitListExprs, by default, has downsides:<br>
<br>
- It may be surprising and undesirable for some RecursiveASTVisitor clients that they see the same nodes visited twice<br>
- 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).<br>
<br>
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 ?</blockquote><div><br></div><div>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.</div><div><br></div><div>Example of why our current AST representation is broken:</div><div><br></div><div>struct S { void (*p)(int); };</div><div>template<typename T> void f(T);</div><div>void g(int);</div><div>S s = { f, .p = g }; // neither syntactic nor semantic form contains a reference to f<int>.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> On Feb 4, 2015, at 6:29 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
><br>
> Author: djasper<br>
> Date: Wed Feb 4 08:29:47 2015<br>
> New Revision: 228144<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=228144&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=228144&view=rev</a><br>
> Log:<br>
> Rewrite r228138 to be somewhat saner.<br>
><br>
> While probably technically correct, the solution r228138 was quite hard<br>
> to read/understand. This should be simpler.<br>
><br>
> Also added a test to ensure that we are still visiting the syntactic form<br>
> as well.<br>
><br>
> Modified:<br>
> cfe/trunk/include/clang/AST/RecursiveASTVisitor.h<br>
> cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=228144&r1=228143&r2=228144&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=228144&r1=228143&r2=228144&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)<br>
> +++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h Wed Feb 4 08:29:47 2015<br>
> @@ -2032,20 +2032,21 @@ DEF_TRAVERSE_STMT(CXXStaticCastExpr, {<br>
> // to the syntactic form.<br>
> template <typename Derived><br>
> bool RecursiveASTVisitor<Derived>::TraverseInitListExpr(InitListExpr *S) {<br>
> - if (InitListExpr *Syn = S->getSyntacticForm())<br>
> - S = Syn;<br>
> - TRY_TO(WalkUpFromInitListExpr(S));<br>
> - // All we need are the default actions. FIXME: use a helper function.<br>
> - for (Stmt::child_range range = S->children(); range; ++range) {<br>
> - TRY_TO(TraverseStmt(*range));<br>
> - }<br>
> - if (InitListExpr *Syn = S->getSemanticForm()) {<br>
> + InitListExpr *Syn = S->isSemanticForm() ? S->getSyntacticForm() : S;<br>
> + if (Syn) {<br>
> TRY_TO(WalkUpFromInitListExpr(Syn));<br>
> // All we need are the default actions. FIXME: use a helper function.<br>
> for (Stmt::child_range range = Syn->children(); range; ++range) {<br>
> TRY_TO(TraverseStmt(*range));<br>
> }<br>
> }<br>
> + InitListExpr *Sem = S->isSemanticForm() ? S : S->getSemanticForm();<br>
> + if (Sem) {<br>
> + TRY_TO(WalkUpFromInitListExpr(Sem));<br>
> + for (Stmt::child_range range = Sem->children(); range; ++range) {<br>
> + TRY_TO(TraverseStmt(*range));<br>
> + }<br>
> + }<br>
> return true;<br>
> }<br>
><br>
><br>
> Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp?rev=228144&r1=228143&r2=228144&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp?rev=228144&r1=228143&r2=228144&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp (original)<br>
> +++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp Wed Feb 4 08:29:47 2015<br>
> @@ -3148,6 +3148,8 @@ TEST(InitListExpression, MatchesInitList<br>
> "void f();"<br>
> "S s[1] = { &f };",<br>
> declRefExpr(to(functionDecl(hasName("f"))))));<br>
> + EXPECT_TRUE(<br>
> + matches("int i[1] = {42, [0] = 43};", integerLiteral(equals(42))));<br>
> }<br>
><br>
> TEST(UsingDeclaration, MatchesUsingDeclarations) {<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>