r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 6 17:22:08 PDT 2017
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).
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 LexicallyOrderedRecursiveASTVisitor
>
> 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/LexicallyOrderedRecursiveASTVisitorTest.cpp
> (original)
> +++ cfe/trunk/unittests/Tooling/LexicallyOrderedRecursiveASTVisitorTest.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/20170906/e3401389/attachment-0001.html>
More information about the cfe-commits
mailing list