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