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.
Details
Details
Diff Detail
Diff Detail
- Build Status
Buildable 9686 Build 9686: arc lint + arc unit
Event Timeline
| include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h | ||
|---|---|---|
| 68 | I don't think you need this since getDerived in RecursiveASTVisitor is already public. | |
| 69 | Why exactly is this template struct needed? | |
| include/clang/AST/RecursiveASTVisitor.h | ||
| 3212 ↗ | (On Diff #112843) | Getting rid of undef is not ideal. You might want to extract these macros into one .def file that's included by both RecursiveASTVisitor.h and the LexicallyOrdered one. |
| include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h | ||
|---|---|---|
| 68 | has_same_member_pointer and getDerived are used by the TRAVERSE_STMT macro, which should be the right one to use here. | |
| include/clang/AST/RecursiveASTVisitor.h | ||
| 3212 ↗ | (On Diff #112843) | Ok, I will do that |
Comment Actions
Ah I see,
then you should something completely different (forget the header).
Please extract this code into a protected function in RecursiveASTVisitor:
for (Stmt *SubStmt : Children)
if (!TRAVERSE_STMT_BASE(Stmt, Stmt, SubStmt, Queue))
return false;And reuse it in this class. Then you won't have to use any macros or related template magic here.
Comment Actions
This works as well I think
| include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h | ||
|---|---|---|
| 63 | Do you still need the using here? | |
Comment Actions
The previous version didn't call TraverseDecl of the derived class, this is fixed now.
The public getDerived.TraverseStmt() does not accept a DataRecursionQueue, so this also could not be used (I think)
I used the wrapper TraverseStmtBase, which should behave exactly as the method that originally traverses CXXOperatorCallExpr
| include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h | ||
|---|---|---|
| 63 | removed it now as it's only used once | |
| include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h | ||
|---|---|---|
| 150 | For ++ and -- you can see whether its prefix or postfix by looking at the number of arguments. If there's one argument, then ++ and -- are prefix. Otherwise, they're postfix. | |
Comment Actions
The fact that TraverseCXXOperatorCallExpr can't call its super TraverseCXXOperatorCallExpr makes the current solution kind of broken. The super implementation of TraverseCXXOperatorCallExpr is responsible for dispatch to WalkUp##STMT functions which actually call VisitCXXOperatorCallExpr which our current visitor will never call because of the custom "override". I would like to suggest an alternative approach that I think solves the problem more "elegantly":
Modify the DEF_TRAVERSE_STMT macro in RecursiveASTVisitor.h call to a wrapper function to get the children:
class RecursiveASTVisitor {
public:
Stmt::child_range getStmtChildren(Stmt *S) {
return S->children();
}
...
#define DEF_TRAVERSE_STMT(STMT, CODE) \
template <typename Derived> \
bool RecursiveASTVisitor<Derived>::Traverse##STMT( \
STMT *S, DataRecursionQueue *Queue) { \
bool ShouldVisitChildren = true; \
bool ReturnValue = true; \
if (!getDerived().shouldTraversePostOrder()) \
TRY_TO(WalkUpFrom##STMT(S)); \
{ CODE; } \
if (ShouldVisitChildren) { \
for (Stmt *SubStmt : getDerived().getStmtChildren(S)) { \ // The only change is on this line.
TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(SubStmt); \
} \
} \
if (!Queue && ReturnValue && getDerived().shouldTraversePostOrder()) \
TRY_TO(WalkUpFrom##STMT(S)); \
return ReturnValue; \
}
...
};Then you could provide an "override" in the LexicallyOrderedRecursiveASTVisitor that adjusts children just for CXXOperatorCallExpr. Something like this should work:
class LexicallyOrderedRecursiveASTVisitor {
public:
...
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;
}
...
};WDYT?
Sorry about not seeing this earlier. Thanks for your patience!
Comment Actions
Yeah, this seems to be the best solution for this. I think I ran into the same issue when working on the StmtDataCollector - basically there can only be two Traverse*, one in the Derived class and the other one needs to do all the magic with walking the specialisation hierarchy.
Do you still need the using here?