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

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas marked 2 inline comments as done.
eduucaldas added inline comments.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:1
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
----------------
I find this name too general. 
We are testing here the behavior of TraverseStmt specifically, perhaps `TraverseStmt.cpp` would be a more appropriate name


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



================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:81
+
+    bool TraverseIntegerLiteral(IntegerLiteral *E) {
+      recordCallback(__func__, E);
----------------
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


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:124
+TraverseIntegerLiteral IntegerLiteral(3)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
----------------
Good! Here the post order is still calling WalkUpFrom even though our Traverse doesn't call it in IntegerLiteral!


================
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);
----------------
> 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.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:153
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E);
----------------
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


================
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
----------------
I think we spotted something funny here. RAV didn't call our overriden TraverseBinaryOperator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82485