Page MenuHomePhabricator

[AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order
Needs ReviewPublic

Authored by johannes on Sep 9 2017, 3:46 AM.

Details

Reviewers
arphaman
rsmith
Summary

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.

Event Timeline

johannes created this revision.Sep 9 2017, 3:46 AM
klimek added a subscriber: klimek.Sep 11 2017, 12:56 AM
klimek added inline comments.
include/clang/AST/RecursiveASTVisitor.h
334

Why do we need to swap for calls?

johannes added inline comments.Sep 11 2017, 7:02 AM
include/clang/AST/RecursiveASTVisitor.h
334

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.

rsmith added inline comments.Sep 11 2017, 3:34 PM
include/clang/AST/RecursiveASTVisitor.h
327

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.

334

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.

340–341

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.

johannes updated this revision to Diff 115018.Sep 13 2017, 5:56 AM

use CXXOperatorCallExpr::isPrefixOp() to determine whether it's infix or postfix
directly traverse statement children instead of copying

johannes added inline comments.Sep 13 2017, 5:58 AM
include/clang/AST/RecursiveASTVisitor.h
327

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.

334

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.