Re: r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
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
wrote:
> Richard Smith 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 getStmtChildren(CXXOperatorCallExpr *CE) {
> >> +SmallVector 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)
> >> +
Re: r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
Richard Smith 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 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 getStmtChildren(CXXOperatorCallExpr *CE) {
>> +SmallVector 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
>>
Re: r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
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@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 getStmtChildren(CXXOperatorCallExpr *CE) {
> +SmallVector 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
>struct has_same_member_pointer_type : std::false_type {};
> @@ -2084,7 +2086,7 @@ DEF_TRAVERSE_DECL(ParmVarDecl, {
>TRY_TO(WalkUpFrom##STMT(S));
> \
> { CODE; }
> \
> if (Sh
r312633 - [AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
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/LexicallyOrderedRecursiveASTVisitorTest.cpp
Modified: cfe/trunk/include/clang/AST/LexicallyOrderedRecursiveASTVisitor.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 getStmtChildren(CXXOperatorCallExpr *CE) {
+SmallVector 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
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/LexicallyOrderedRecursiveASTVisitorTest.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 {
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
