<div dir="ltr">It's a good idea to merge the two. I'll work on moving the ObjC traversal change when I get the time.<div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for the quick patches Johannes!</div><div class="gmail_extra"><br><div class="gmail_quote">On 9 September 2017 at 12:03, Johannes Altmanninger <span dir="ltr"><<a href="mailto:aclopte@gmail.com" target="_blank">aclopte@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> writes:<br>
<br>
> I am extremely uncomfortable about the direction this patch series is going.<br>
><br>
> We have had two different RecursiveASTVisitors before (RecursiveASTVisitor<br>
> and DataRecursiveASTVisitor), and it was a maintenance nightmare:<br>
> frequently changes would be made to one of them and missed in the other<br>
> one, resulting in hard to track down bugs, as well as redundant development<br>
> effort making changes to both.<br>
><br>
> Back when this was simply deriving from RecursiveASTVisitor and not<br>
> changing much, LexicallyOrderedRecursiveASTVi<wbr>sitor seemed like it wouldn't<br>
> suffer from the same problem, but it's becoming increasingly clear that<br>
> that simply isn't going to be the case long-term. And worse, there seems to<br>
> be absolutely no purpose in having two different visitors here -- the<br>
> existing users of RecursiveASTVisitor generally don't care about visitation<br>
> order.<br>
><br>
> Also, we can play the "what if two people did this" game -- adding RAV<br>
> functionality in derived classes doesn't compose well. (If post-order<br>
> traversal were a derived class, you couldn't request a lexically-ordered<br>
> post-order traversal, and so on.)<br>
><br>
> Therefore, I'd like to suggest that you stop making changes to<br>
> LexicallyOrderedRecursiveASTVi<wbr>sitor and instead start to fold its<br>
> functionality back into RecursiveASTVisitor, as follows:<br>
><br>
> * in cases where there is no reason for RAV to visit in non-lexical order,<br>
> change it to visit in lexical order<br>
> * if there are any cases where there *is* a reason for non-lexical<br>
> visitation, add a bool function to it so the derived class can request<br>
> lexical / non-lexical order (like the existing shouldTraversePostOrder /<br>
> shouldVisitImplicitCode / ... functions).<br>
<br>
</div></div>Ok, makes sense.<br>
<br>
+Alex so you are aware.<br>
<br>
I have created two revisions to move my changes to RecursiveASTVisitor,<br>
this should not break anything. I left the tests in<br>
LexicallyOrderedRecursiveASTVi<wbr>sitorTest for now because it saves some<br>
code duplication, but let's see, if we merge all the changes into<br>
RecursiveASTVisitor, then the tests will naturally follow.<br>
<br>
<a href="https://reviews.llvm.org/D37662" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37662</a><br>
<a href="https://reviews.llvm.org/D37663" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37663</a><br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> On 6 September 2017 at 06:12, Johannes Altmanninger via cfe-commits <<br>
> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
>> Author: krobelus<br>
>> Date: Wed Sep 6 06:12:11 2017<br>
>> New Revision: 312633<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=312633&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=312633&view=rev</a><br>
>> Log:<br>
>> [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVi<wbr>sitor<br>
>><br>
>> Summary:<br>
>> This affects overloaded operators, which are represented by a<br>
>> CXXOperatorCallExpr whose first child is always a DeclRefExpr referring to<br>
>> the<br>
>> operator. For infix, postfix and call operators we want the first argument<br>
>> to be traversed before the operator.<br>
>><br>
>> Reviewers: arphaman<br>
>><br>
>> Subscribers: klimek, cfe-commits<br>
>><br>
>> Differential Revision: <a href="https://reviews.llvm.org/D37200" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37200</a><br>
>><br>
>> Modified:<br>
>> cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h<br>
>> cfe/trunk/include/clang/AST/<wbr>RecursiveASTVisitor.h<br>
>> cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<br>
>> sitorTest.cpp<br>
>><br>
>> Modified: cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<br>
>> sitor.h<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/AST/</a><br>
>> LexicallyOrderedRecursiveASTVi<wbr>sitor.h?rev=312633&r1=312632&<br>
>> r2=312633&view=diff<br>
>> ==============================<wbr>==============================<br>
>> ==================<br>
>> --- cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h<br>
>> (original)<br>
>> +++ cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h Wed<br>
>> Sep 6 06:12:11 2017<br>
>> @@ -113,6 +113,33 @@ public:<br>
>><br>
>> bool shouldTraverseTemplateArgument<wbr>sBeforeDecl() const { return true; }<br>
>><br>
>> + Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }<br>
>> +<br>
>> + SmallVector<Stmt *, 8> getStmtChildren(<wbr>CXXOperatorCallExpr *CE) {<br>
>> + SmallVector<Stmt *, 8> Children(CE->children());<br>
>> + bool Swap;<br>
>> + // Switch the operator and the first operand for all infix and postfix<br>
>> + // operations.<br>
>> + switch (CE->getOperator()) {<br>
>> + case OO_Arrow:<br>
>> + case OO_Call:<br>
>> + case OO_Subscript:<br>
>> + Swap = true;<br>
>> + break;<br>
>> + case OO_PlusPlus:<br>
>> + case OO_MinusMinus:<br>
>> + // These are postfix unless there is exactly one argument.<br>
>> + Swap = Children.size() != 2;<br>
>> + break;<br>
>> + default:<br>
>> + Swap = CE->isInfixBinaryOp();<br>
>> + break;<br>
>> + }<br>
>> + if (Swap && Children.size() > 1)<br>
>> + std::swap(Children[0], Children[1]);<br>
>> + return Children;<br>
>> + }<br>
>> +<br>
>> private:<br>
>> bool TraverseAdditionalLexicallyNes<wbr>tedDeclarations() {<br>
>> // FIXME: Ideally the gathered declarations and the declarations in<br>
>> the<br>
>><br>
>> Modified: cfe/trunk/include/clang/AST/<wbr>RecursiveASTVisitor.h<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/</a><br>
>> clang/AST/RecursiveASTVisitor.<wbr>h?rev=312633&r1=312632&r2=<wbr>312633&view=diff<br>
>> ==============================<wbr>==============================<br>
>> ==================<br>
>> --- cfe/trunk/include/clang/AST/<wbr>RecursiveASTVisitor.h (original)<br>
>> +++ cfe/trunk/include/clang/AST/<wbr>RecursiveASTVisitor.h Wed Sep 6 06:12:11<br>
>> 2017<br>
>> @@ -315,6 +315,8 @@ public:<br>
>><br>
>> // ---- Methods on Stmts ----<br>
>><br>
>> + Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }<br>
>> +<br>
>> private:<br>
>> template<typename T, typename U><br>
>> struct has_same_member_pointer_type : std::false_type {};<br>
>> @@ -2084,7 +2086,7 @@ DEF_TRAVERSE_DECL(ParmVarDecl, {<br>
>> TRY_TO(WalkUpFrom##STMT(S));<br>
>> \<br>
>> { CODE; }<br>
>> \<br>
>> if (ShouldVisitChildren) {<br>
>> \<br>
>> - for (Stmt *SubStmt : S->children()) {<br>
>> \<br>
>> + for (Stmt * SubStmt : getDerived().getStmtChildren(<wbr>S)) {<br>
>> \<br>
>> TRY_TO_TRAVERSE_OR_ENQUEUE_<wbr>STMT(SubStmt);<br>
>> \<br>
>> }<br>
>> \<br>
>> }<br>
>> \<br>
>><br>
>> Modified: cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<br>
>> sitorTest.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/unittests/<wbr>Tooling/</a><br>
>> LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp?rev=312633&r1=<br>
>> 312632&r2=312633&view=diff<br>
>> ==============================<wbr>==============================<br>
>> ==================<br>
>> --- cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp<br>
>> (original)<br>
>> +++ cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp<br>
>> Wed Sep 6 06:12:11 2017<br>
>> @@ -21,9 +21,10 @@ class LexicallyOrderedDeclVisitor<br>
>> : public LexicallyOrderedRecursiveASTVi<wbr>sitor<<wbr>LexicallyOrderedDeclVisitor><br>
>> {<br>
>> public:<br>
>> LexicallyOrderedDeclVisitor(<wbr>DummyMatchVisitor &Matcher,<br>
>> - const SourceManager &SM, bool EmitIndices)<br>
>> + const SourceManager &SM, bool<br>
>> EmitDeclIndices,<br>
>> + bool EmitStmtIndices)<br>
>> : LexicallyOrderedRecursiveASTVi<wbr>sitor(SM), Matcher(Matcher),<br>
>> - EmitIndices(EmitIndices) {}<br>
>> + EmitDeclIndices(<wbr>EmitDeclIndices), EmitStmtIndices(<wbr>EmitStmtIndices)<br>
>> {}<br>
>><br>
>> bool TraverseDecl(Decl *D) {<br>
>> TraversalStack.push_back(D);<br>
>> @@ -32,31 +33,43 @@ public:<br>
>> return true;<br>
>> }<br>
>><br>
>> + bool TraverseStmt(Stmt *S);<br>
>> +<br>
>> bool VisitNamedDecl(const NamedDecl *D);<br>
>> + bool VisitDeclRefExpr(const DeclRefExpr *D);<br>
>><br>
>> private:<br>
>> DummyMatchVisitor &Matcher;<br>
>> - bool EmitIndices;<br>
>> + bool EmitDeclIndices, EmitStmtIndices;<br>
>> unsigned Index = 0;<br>
>> llvm::SmallVector<Decl *, 8> TraversalStack;<br>
>> };<br>
>><br>
>> class DummyMatchVisitor : public ExpectedLocationVisitor<<wbr>DummyMatchVisitor><br>
>> {<br>
>> - bool EmitIndices;<br>
>> + bool EmitDeclIndices, EmitStmtIndices;<br>
>><br>
>> public:<br>
>> - DummyMatchVisitor(bool EmitIndices = false) : EmitIndices(EmitIndices)<br>
>> {}<br>
>> + DummyMatchVisitor(bool EmitDeclIndices = false, bool EmitStmtIndices =<br>
>> false)<br>
>> + : EmitDeclIndices(<wbr>EmitDeclIndices), EmitStmtIndices(<wbr>EmitStmtIndices)<br>
>> {}<br>
>> bool VisitTranslationUnitDecl(<wbr>TranslationUnitDecl *TU) {<br>
>> const ASTContext &Context = TU->getASTContext();<br>
>> const SourceManager &SM = Context.getSourceManager();<br>
>> - LexicallyOrderedDeclVisitor SubVisitor(*this, SM, EmitIndices);<br>
>> + LexicallyOrderedDeclVisitor SubVisitor(*this, SM, EmitDeclIndices,<br>
>> + EmitStmtIndices);<br>
>> SubVisitor.TraverseDecl(TU);<br>
>> return false;<br>
>> }<br>
>><br>
>> - void match(StringRef Path, const Decl *D) { Match(Path,<br>
>> D->getLocStart()); }<br>
>> + template <class T> void match(StringRef Path, const T *D) {<br>
>> + Match(Path, D->getLocStart());<br>
>> + }<br>
>> };<br>
>><br>
>> +bool LexicallyOrderedDeclVisitor::<wbr>TraverseStmt(Stmt *S) {<br>
>> + Matcher.match("overridden TraverseStmt", S);<br>
>> + return LexicallyOrderedRecursiveASTVi<wbr>sitor::TraverseStmt(S);<br>
>> +}<br>
>> +<br>
>> bool LexicallyOrderedDeclVisitor::<wbr>VisitNamedDecl(const NamedDecl *D) {<br>
>> std::string Path;<br>
>> llvm::raw_string_ostream OS(Path);<br>
>> @@ -73,7 +86,16 @@ bool LexicallyOrderedDeclVisitor::<wbr>VisitN<br>
>> if (isa<DeclContext>(D) or isa<TemplateDecl>(D))<br>
>> OS << "/";<br>
>> }<br>
>> - if (EmitIndices)<br>
>> + if (EmitDeclIndices)<br>
>> + OS << "@" << Index++;<br>
>> + Matcher.match(OS.str(), D);<br>
>> + return true;<br>
>> +}<br>
>> +<br>
>> +bool LexicallyOrderedDeclVisitor::<wbr>VisitDeclRefExpr(const DeclRefExpr *D)<br>
>> {<br>
>> + std::string Name = D->getFoundDecl()-><wbr>getNameAsString();<br>
>> + llvm::raw_string_ostream OS(Name);<br>
>> + if (EmitStmtIndices)<br>
>> OS << "@" << Index++;<br>
>> Matcher.match(OS.str(), D);<br>
>> return true;<br>
>> @@ -160,4 +182,46 @@ template <class U, class = void> class C<br>
>> EXPECT_TRUE(Visitor.runOver(<wbr>Source));<br>
>> }<br>
>><br>
>> +TEST(<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor, VisitCXXOperatorCallExpr) {<br>
>> + StringRef Source = R"(<br>
>> +struct S {<br>
>> + S &operator+(S&);<br>
>> + S *operator->();<br>
>> + S &operator++();<br>
>> + S operator++(int);<br>
>> + void operator()(int, int);<br>
>> + void operator[](int);<br>
>> + void f();<br>
>> +};<br>
>> +S a, b, c;<br>
>> +<br>
>> +void test() {<br>
>> + a = b + c;<br>
>> + a->f();<br>
>> + a(1, 2);<br>
>> + b[0];<br>
>> + ++a;<br>
>> + b++;<br>
>> +}<br>
>> +)";<br>
>> + DummyMatchVisitor Visitor(/*EmitDeclIndices=*/<wbr>false,<br>
>> + /*EmitStmtIndices=*/true);<br>
>> + // There are two overloaded operators that start at this point<br>
>> + // This makes sure they are both traversed using the overridden<br>
>> + // TraverseStmt, as the traversal is implemented by us for<br>
>> + // CXXOperatorCallExpr.<br>
>> + Visitor.ExpectMatch("<wbr>overridden TraverseStmt", 14, 3, 2);<br>
>> + Visitor.ExpectMatch("a@0", 14, 3);<br>
>> + Visitor.ExpectMatch("operator=<wbr>@1", 14, 5);<br>
>> + Visitor.ExpectMatch("b@2", 14, 7);<br>
>> + Visitor.ExpectMatch("operator+<wbr>@3", 14, 9);<br>
>> + Visitor.ExpectMatch("c@4", 14, 11);<br>
>> + Visitor.ExpectMatch("operator-<wbr>>@6", 15, 4);<br>
>> + Visitor.ExpectMatch("operator(<wbr>)@8", 16, 4);<br>
>> + Visitor.ExpectMatch("operator[<wbr>]@10", 17, 4);<br>
>> + Visitor.ExpectMatch("operator+<wbr>+@11", 18, 3);<br>
>> + Visitor.ExpectMatch("operator+<wbr>+@14", 19, 4);<br>
>> + EXPECT_TRUE(Visitor.runOver(<wbr>Source));<br>
>> +}<br>
>> +<br>
>> } // end anonymous namespace<br>
>><br>
>><br>
>> ______________________________<wbr>_________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
>><br>
</div></div></blockquote></div><br></div></div></div>