r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
Johannes Altmanninger via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 9 04:03:04 PDT 2017
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 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
>>
More information about the cfe-commits
mailing list