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