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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
