[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 marked 5 inline comments as done.
gribozavr2 added inline comments.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:16
+template <typename Derived>
+class RecordingVisitorBase : public TestVisitor<Derived> {
+  bool VisitPostOrder;
----------------
ymandel wrote:
> Add class comment?
Added.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:17
+class RecordingVisitorBase : public TestVisitor<Derived> {
+  bool VisitPostOrder;
+
----------------
eduucaldas wrote:
> ymandel wrote:
> > Consider using an enum rather than a bool.
> Agreed.
> This would avoid all the /*VisitPostOrder=*/false
> 
Changed to enum.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:81
+
+    bool TraverseIntegerLiteral(IntegerLiteral *E) {
+      recordCallback(__func__, E);
----------------
eduucaldas wrote:
> E for Expr? Ok, Expr is base class of IntegerLiteral.
> I propose to use either: 
> S for Stmt, it  is a more homogeneus name and **also** a base class of IntegerLiteral
> Or 
> IL for IntegerLiteral, and then we stick with this convention
Changed to a more specific abbreviation everywhere I could find.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:148-161
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S);
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E);
----------------
eduucaldas wrote:
> > E for Expr? Ok, Expr is base class of IntegerLiteral.
> > I propose to use either:
> > S for Stmt, it is a more homogeneus name and also a base class of IntegerLiteral
> > Or
> > IL for IntegerLiteral, and then we stick with this convention
> 
> Here it gets even more confusing.
Changed the name to `IL`.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:153
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E);
----------------
eduucaldas wrote:
> I think overriding WalkUpFromDerived when you already have WalkUpFromStmt doesn't bring much value.
> In the case of fully derived AST nodes we just get the repeated information about the type of this node, e.g. 
> WalkUpFromIntegerLiteral IntegerLiteral(x) 
> instead of
> WalkUpFromStmt IntegerLiteral(x)
>  
> In the case of intermediate AST nodes, as WalkUpFromExpr, we get some information but do we need that?
> Here for instance, the information we get is: 
> WalkUpFromExpr Node => Node is an Expr
> WalkUpFromStmt Node => Node is a Stmt
> I don't think this information is very valuable
I added these overrides not to collect more information, but to get more coverage. It is true that if we look carefully at the implementation we can probably infer that WalkUpFrom chaining works fine. I'm adding tests to ensure that it continues to work correctly in future. It would be nice if we could factor out a separate test that shows just that chaining logic, but I don't see how to easily do it. WalkUpFrom methods are called from a bunch of places depending on what Traverse methods are overridden, whether we are in the data recursion optimization, and whether we are in the post-order traversal mode. Those focused tests would have to define the same combinations of callbacks in RecursiveASTVisitor as these tests here.



================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:300-309
+WalkUpFromStmt CompoundStmt
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
----------------
eduucaldas wrote:
> I think we spotted something funny here. RAV didn't call our overriden TraverseBinaryOperator.
I added a FIXME that explains that this is a potential bug.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82485/new/

https://reviews.llvm.org/D82485