r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
Alex L via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 11 03:38:30 PDT 2017
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 <aclopte at gmail.com>
wrote:
> Richard Smith <richard at 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 at 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 at 0", 14, 3);
> >> + Visitor.ExpectMatch("operator=@1", 14, 5);
> >> + Visitor.ExpectMatch("b at 2", 14, 7);
> >> + Visitor.ExpectMatch("operator+ at 3", 14, 9);
> >> + Visitor.ExpectMatch("c at 4", 14, 11);
> >> + Visitor.ExpectMatch("operator->@6", 15, 4);
> >> + Visitor.ExpectMatch("operator()@8", 16, 4);
> >> + Visitor.ExpectMatch("operator[]@10", 17, 4);
> >> + Visitor.ExpectMatch("operator++ at 11", 18, 3);
> >> + Visitor.ExpectMatch("operator++ at 14", 19, 4);
> >> + EXPECT_TRUE(Visitor.runOver(Source));
> >> +}
> >> +
> >> } // end anonymous namespace
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170911/8adf18cd/attachment-0001.html>
More information about the cfe-commits
mailing list