<div dir="ltr">I am extremely uncomfortable about the direction this patch series is going.<div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.)</div><div><br></div><div>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:</div><div><br></div><div> * in cases where there is no reason for RAV to visit in non-lexical order, change it to visit in lexical order</div><div> * 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).</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 6 September 2017 at 06:12, Johannes Altmanninger via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 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<wbr>sitorTest.cpp<br>
<br>
Modified: cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h?rev=312633&r1=312632&r2=312633&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h?rev=312633&r1=312632&<wbr>r2=312633&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h (original)<br>
+++ cfe/trunk/include/clang/AST/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitor.h Wed 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 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/clang/AST/RecursiveASTVisitor.h?rev=312633&r1=312632&r2=312633&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/AST/RecursiveASTVisitor.<wbr>h?rev=312633&r1=312632&r2=<wbr>312633&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<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 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>
{ CODE; } \<br>
if (ShouldVisitChildren) { \<br>
- for (Stmt *SubStmt : S->children()) { \<br>
+ for (Stmt * SubStmt : getDerived().getStmtChildren(<wbr>S)) { \<br>
TRY_TO_TRAVERSE_OR_ENQUEUE_<wbr>STMT(SubStmt); \<br>
} \<br>
} \<br>
<br>
Modified: cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/LexicallyOrderedRecursiveASTVisitorTest.cpp?rev=312633&r1=312632&r2=312633&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/unittests/<wbr>Tooling/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp?rev=312633&r1=<wbr>312632&r2=312633&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp (original)<br>
+++ cfe/trunk/unittests/Tooling/<wbr>LexicallyOrderedRecursiveASTVi<wbr>sitorTest.cpp Wed Sep 6 06:12:11 2017<br>
@@ -21,9 +21,10 @@ class LexicallyOrderedDeclVisitor<br>
: public LexicallyOrderedRecursiveASTVi<wbr>sitor<<wbr>LexicallyOrderedDeclVisitor> {<br>
public:<br>
LexicallyOrderedDeclVisitor(<wbr>DummyMatchVisitor &Matcher,<br>
- const SourceManager &SM, bool EmitIndices)<br>
+ const SourceManager &SM, bool EmitDeclIndices,<br>
+ bool EmitStmtIndices)<br>
: LexicallyOrderedRecursiveASTVi<wbr>sitor(SM), Matcher(Matcher),<br>
- EmitIndices(EmitIndices) {}<br>
+ EmitDeclIndices(<wbr>EmitDeclIndices), EmitStmtIndices(<wbr>EmitStmtIndices) {}<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>
- bool EmitIndices;<br>
+ bool EmitDeclIndices, EmitStmtIndices;<br>
<br>
public:<br>
- DummyMatchVisitor(bool EmitIndices = false) : EmitIndices(EmitIndices) {}<br>
+ DummyMatchVisitor(bool EmitDeclIndices = false, bool EmitStmtIndices = false)<br>
+ : EmitDeclIndices(<wbr>EmitDeclIndices), EmitStmtIndices(<wbr>EmitStmtIndices) {}<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, 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>
+ 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>
</blockquote></div><br></div>