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