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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


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