[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
johannes added inline comments.
Comment at: include/clang/AST/RecursiveASTVisitor.h:327
+
+ SmallVector getStmtChildren(CXXOperatorCallExpr *CE) {
+SmallVector Children(CE->children());
rsmith wrote:
> The copy here is more expensive than I'd like. Instead of returning a list of
> children, please make this function *traverse* the children (which is what
> its one and only caller is going to do anyway) to avoid the copy.
I wasn't sure whether there is a way to do this in a function without writing a
custom iterator that checks whether it should be swapped; I used a different
approach.
Comment at: include/clang/AST/RecursiveASTVisitor.h:334
+case OO_Arrow:
+case OO_Call:
+case OO_Subscript:
rsmith wrote:
> johannes wrote:
> > klimek wrote:
> > > Why do we need to swap for calls?
> > The idea is that the opening parenthesis/bracket comes after the caller,
> > for example in this call with two arguments.
> >
> > ```
> > `-CXXOperatorCallExpr
> > |-ImplicitCastExpr
> > | `-DeclRefExpr operator()
> > |-DeclRefExpr caller
> > |-IntegerLiteral
> > `-IntegerLiteral
> > ```
> >
> > Of course we fail to capture the fact that there is also a closing
> > parenthesis or bracket. So this is no perfect solution.
> > Of course we fail to capture the fact that there is also a closing
> > parenthesis or bracket.
>
> One way we could handle this would be to effectively order by the start
> location for preorder traversal, and by the end location for postorder
> traversal. So for a case like your `caller(1,2)`, we would visit
> `CXXOperatorCallExpr`, `caller`, `operator()`, `1`, then `2` when doing
> preorder visitation, and `caller`, `1`, `2`, `operator()`, then
> `CXXOperatorCallExpr` when doing postorder visitation (because the notional
> source location for the `operator()` invocation extends across both `1` and
> `2` subexpressions).
>
> But I think it's probably not worthwhile to go to this level of detail, and
> treating the `(` as the location of the `operator()` call and ignoring the
> `)` seems like a reasonable approach to me.
Yeah, we can do that if someone needs it.
Another way would be to model the operator as parent of the operands, although
I assume that this would not compose well.
I don't expect it to be feasible but I imagine CXXOperatorCallExpr could also
inherit from DeclRefExpr.
https://reviews.llvm.org/D37663
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
johannes updated this revision to Diff 115018.
johannes added a comment.
use CXXOperatorCallExpr::isPrefixOp() to determine whether it's infix or postfix
directly traverse statement children instead of copying
https://reviews.llvm.org/D37663
Files:
include/clang/AST/ExprCXX.h
include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
include/clang/AST/RecursiveASTVisitor.h
lib/AST/ExprCXX.cpp
Index: lib/AST/ExprCXX.cpp
===
--- lib/AST/ExprCXX.cpp
+++ lib/AST/ExprCXX.cpp
@@ -41,6 +41,12 @@
}
}
+bool CXXOperatorCallExpr::isPrefixOp() const {
+ return getNumArgs() == 1 && getOperator() != OO_Call &&
+ getOperator() != OO_Arrow;
+}
+
+
bool CXXTypeidExpr::isPotentiallyEvaluated() const {
if (isTypeOperand())
return false;
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -315,8 +315,6 @@
// Methods on Stmts
- Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }
-
private:
template
struct has_same_member_pointer_type : std::false_type {};
@@ -2080,15 +2078,26 @@
TRY_TO(WalkUpFrom##STMT(S)); \
{ CODE; } \
if (ShouldVisitChildren) { \
- for (Stmt * SubStmt : getDerived().getStmtChildren(S)) { \
-TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(SubStmt); \
+ auto ChildIterator = S->child_begin(); \
+ /* Make the operator come second for infix and postfix overloaded\
+ * operators. */ \
+ if (auto *CE = dyn_cast(S)) { \
+if (CE->isInfixBinaryOp() || !CE->isPrefixOp()) { \
+ Stmt *Operator = *ChildIterator; \
+ TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(*++ChildIterator); \
+ TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(Operator); \
+ ++ChildIterator; \
+} \
+ }\
+ for (auto ChildEnd = S->child_end(); ChildIterator != ChildEnd; \
+ ++ChildIterator) { \
+TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(*ChildIterator); \
}\
} \
if (!Queue && ReturnValue && getDerived().shouldTraversePostOrder()) \
TRY_TO(WalkUpFrom##STMT(S)); \
return ReturnValue;\
}
-
DEF_TRAVERSE_STMT(GCCAsmStmt, {
TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(S->getAsmString());
for (unsigned I = 0, E = S->getNumInputs(); I < E; ++I) {
Index: include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
===
--- include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
+++ include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
@@ -111,33 +111,6 @@
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
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -87,6 +87,8 @@
/// \brief Is this written as an infix binary operator?
bool isInfixBinaryOp() const;
+ bool isPrefixOp() const;
+
/// \brief Returns the location of the operator symbol in the expression.
///
/// When \c getOperator()==O
[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
rsmith added inline comments.
Comment at: include/clang/AST/RecursiveASTVisitor.h:327
+
+ SmallVector getStmtChildren(CXXOperatorCallExpr *CE) {
+SmallVector Children(CE->children());
The copy here is more expensive than I'd like. Instead of returning a list of
children, please make this function *traverse* the children (which is what its
one and only caller is going to do anyway) to avoid the copy.
Comment at: include/clang/AST/RecursiveASTVisitor.h:334
+case OO_Arrow:
+case OO_Call:
+case OO_Subscript:
johannes wrote:
> klimek wrote:
> > Why do we need to swap for calls?
> The idea is that the opening parenthesis/bracket comes after the caller, for
> example in this call with two arguments.
>
> ```
> `-CXXOperatorCallExpr
> |-ImplicitCastExpr
> | `-DeclRefExpr operator()
> |-DeclRefExpr caller
> |-IntegerLiteral
> `-IntegerLiteral
> ```
>
> Of course we fail to capture the fact that there is also a closing
> parenthesis or bracket. So this is no perfect solution.
> Of course we fail to capture the fact that there is also a closing
> parenthesis or bracket.
One way we could handle this would be to effectively order by the start
location for preorder traversal, and by the end location for postorder
traversal. So for a case like your `caller(1,2)`, we would visit
`CXXOperatorCallExpr`, `caller`, `operator()`, `1`, then `2` when doing
preorder visitation, and `caller`, `1`, `2`, `operator()`, then
`CXXOperatorCallExpr` when doing postorder visitation (because the notional
source location for the `operator()` invocation extends across both `1` and `2`
subexpressions).
But I think it's probably not worthwhile to go to this level of detail, and
treating the `(` as the location of the `operator()` call and ignoring the `)`
seems like a reasonable approach to me.
Comment at: include/clang/AST/RecursiveASTVisitor.h:340-341
+case OO_MinusMinus:
+ // These are postfix unless there is exactly one argument.
+ Swap = Children.size() != 2;
+ break;
This would be clearer as `Children.size() == 3`.
However, instead of changing that, please add a
`CXXOperatorCallExpr::isPrefixOp()` function (`getNumArgs() == 1 && Op !=
OO_Call && Op != OO_Arrow`); we should visit the callee after visiting the
first argument if and only if we have a prefix operator.
https://reviews.llvm.org/D37663
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
johannes added inline comments. Comment at: include/clang/AST/RecursiveASTVisitor.h:334 +case OO_Arrow: +case OO_Call: +case OO_Subscript: klimek wrote: > Why do we need to swap for calls? The idea is that the opening parenthesis/bracket comes after the caller, for example in this call with two arguments. ``` `-CXXOperatorCallExpr |-ImplicitCastExpr | `-DeclRefExpr operator() |-DeclRefExpr caller |-IntegerLiteral `-IntegerLiteral ``` Of course we fail to capture the fact that there is also a closing parenthesis or bracket. So this is no perfect solution. https://reviews.llvm.org/D37663 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
klimek added inline comments. Comment at: include/clang/AST/RecursiveASTVisitor.h:334 +case OO_Arrow: +case OO_Call: +case OO_Subscript: Why do we need to swap for calls? https://reviews.llvm.org/D37663 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
johannes created this revision.
This adds a special case for traversing CXXOperatorCallExpr. Its
children are traversed in the order in which they appear in the source
code, so infix and postfix operators are visited after their first
argument.
This behavior was previously only in
LexicallyOrderedRecursiveASTVisitor, moving it here could avoid bugs in
the future. It is still tested in
LexicallyOrderedRecursiveASTVisitorTest.cpp in VisitTemplateDecl.
Clients that somehow rely on the original traversal order will have to
be updated though.
https://reviews.llvm.org/D37663
Files:
include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
include/clang/AST/RecursiveASTVisitor.h
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -315,15 +315,40 @@
// Methods on Stmts
- Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }
-
private:
template
struct has_same_member_pointer_type : std::false_type {};
template
struct has_same_member_pointer_type
: std::true_type {};
+ Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }
+
+ SmallVector getStmtChildren(CXXOperatorCallExpr *CE) {
+SmallVector Children(CE->children());
+bool Swap;
+// Swap 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;
+ }
+
// Traverse the given statement. If the most-derived traverse function takes
a
// data recursion queue, pass it on; otherwise, discard it. Note that the
// first branch of this conditional must compile whether or not the derived
Index: include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
===
--- include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
+++ include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
@@ -111,33 +111,6 @@
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
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -315,15 +315,40 @@
// Methods on Stmts
- Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }
-
private:
template
struct has_same_member_pointer_type : std::false_type {};
template
struct has_same_member_pointer_type
: std::true_type {};
+ Stmt::child_range getStmtChildren(Stmt *S) { return S->children(); }
+
+ SmallVector getStmtChildren(CXXOperatorCallExpr *CE) {
+SmallVector Children(CE->children());
+bool Swap;
+// Swap 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;
+ }
+
// Traverse the given statement. If the most-derived traverse function takes a
// data recursion queue, pass it on; otherwise, discard it. Note that the
// first branch of this conditional must compile whether or not the derived
Index: include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
==
