[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order

[Johannes Altmanninger via Phabricator via cfe-commits]

johannes added inline comments.


================
Comment at: include/clang/AST/RecursiveASTVisitor.h:327
+
+  SmallVector<Stmt *, 8> getStmtChildren(CXXOperatorCallExpr *CE) {
+    SmallVector<Stmt *, 8> 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