Page MenuHomePhabricator

[AST] Traverse CXXOperatorCallExpr in LexicallyOrderedRecursiveASTVisitor
ClosedPublic

Authored by johannes on Aug 27 2017, 3:55 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

johannes created this revision.Aug 27 2017, 3:55 PM
arphaman added inline comments.Aug 28 2017, 3:55 AM
include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
69 ↗(On Diff #112843)

Why exactly is this template struct needed?

75 ↗(On Diff #112843)

I don't think you need this since getDerived in RecursiveASTVisitor is already public.

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.

johannes added inline comments.Aug 28 2017, 4:03 AM
include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
75 ↗(On Diff #112843)

has_same_member_pointer and getDerived are used by the TRAVERSE_STMT macro, which should be the right one to use here.
Also the RecursiveASTVisitor typedef just above.

include/clang/AST/RecursiveASTVisitor.h
3212 ↗(On Diff #112843)

Ok, I will do that

arphaman edited edge metadata.Aug 28 2017, 4:11 AM

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.

johannes updated this revision to Diff 112885.Aug 28 2017, 5:01 AM

use RecursiveASTVisitor::TraverseStmt

This works as well I think

include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
63 ↗(On Diff #112885)

Do you still need the using here?

johannes updated this revision to Diff 112889.Aug 28 2017, 6:19 AM

do call derived TraverseStmt for children of CXXOperatorCallExpr

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 ↗(On Diff #112885)

removed it now as it's only used once

arphaman added inline comments.Aug 28 2017, 6:32 AM
include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
149 ↗(On Diff #112889)

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.

johannes updated this revision to Diff 112894.Aug 28 2017, 6:48 AM

detect prefix/postfix from number of arguments

arphaman added a comment.EditedAug 28 2017, 7:29 AM

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!

johannes updated this revision to Diff 112951.Aug 28 2017, 1:29 PM

use a specialized getStmtChildren to fix the order for CXXOperatorCallExpr

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.

arphaman accepted this revision.Aug 29 2017, 5:25 AM

Lgtm with two requests below

include/clang/AST/RecursiveASTVisitor.h
354 ↗(On Diff #112951)

I don't think you need this function now

2089 ↗(On Diff #112951)

Please avoid deleting this line

This revision is now accepted and ready to land.Aug 29 2017, 5:25 AM
This revision was automatically updated to reflect the committed changes.