Page MenuHomePhabricator

Add tests for sequences of callbacks that RecursiveASTVisitor produces
ClosedPublic

Authored by gribozavr on Jun 24 2020, 11:01 AM.

Details

Summary

These tests show a bug: post-order traversal introduces an extra call to
WalkUp*, that is not present in pre-order traversal. I'm fixing this bug
in a follow-up commit.

Diff Detail

Event Timeline

gribozavr created this revision.Jun 24 2020, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2020, 11:01 AM
ymandel accepted this revision.Jun 24 2020, 12:40 PM
ymandel added inline comments.
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
17

Add class comment?

18

Consider using an enum rather than a bool.

This revision is now accepted and ready to land.Jun 24 2020, 12:40 PM
eduucaldas marked 2 inline comments as done.Jun 25 2020, 1:42 AM
eduucaldas added inline comments.
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
2

I find this name too general.
We are testing here the behavior of TraverseStmt specifically, perhaps TraverseStmt.cpp would be a more appropriate name

18

Agreed.
This would avoid all the /*VisitPostOrder=*/false

82

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

125

Good! Here the post order is still calling WalkUpFrom even though our Traverse doesn't call it in IntegerLiteral!

149–162

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.

154

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

301–310

I think we spotted something funny here. RAV didn't call our overriden TraverseBinaryOperator.

gribozavr2 marked 5 inline comments as done.Jun 25 2020, 4:17 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
17

Added.

18

Changed to enum.

82

Changed to a more specific abbreviation everywhere I could find.

149–162

Changed the name to IL.

154

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.

301–310

I added a FIXME that explains that this is a potential bug.

gribozavr updated this revision to Diff 273302.Jun 25 2020, 4:19 AM

Addressed code review comments.

ymandel accepted this revision.Jun 25 2020, 6:35 AM
eduucaldas accepted this revision.Jun 25 2020, 8:00 AM

Added calls to default implementations.

A more general feedback.
From our conversation one of the issues was that the tests wre only surfacing overriden methods. For instance, whenever we recorded a WalkUpFromExpr, and thus the callback showed up in the test, we actually did not walk up.
Now, in all the test cases we are calling the default implementation. We are not surfacing that WalkUpFrom can not walk up.

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
77

We don't use CallbackName.

108–111

I think you meant to unify recordCallback and recodDefaultImplentation into one, and that's why you had added this __func__ to recordDefaultImplementation.

118

Do we need this this?

gribozavr updated this revision to Diff 273834.Jun 26 2020, 2:09 PM

Removed an unused argument from recordDefaultImplementation.

gribozavr updated this revision to Diff 273835.Jun 26 2020, 2:17 PM

Unified recordCallback and recordDefaultImplementation into one call.

Now, in all the test cases we are calling the default implementation. We are not surfacing that WalkUpFrom can not walk up.

I think we are. The log of callbacks called from the default implementation is indented to the right, so we clearly see what the default implementation does, and what the behavior would have been if we didn't call the default implementation.

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
77

Removed (initially I had a different implementation).

108–111

I was passing __func__ to recordDefaultImplementation for a different reason (I had a different output format initially), but unifying the two functions like you're suggesting makes a lot of sense. Changed the code to do that.

118

Yes, we're calling a method on this from the base class. RecordingVisitorBase::WalkUpFromStmt(S); is implicitly calling a member function, this-> RecordingVisitorBase::WalkUpFromStmt(S);.

eduucaldas accepted this revision.Jun 29 2020, 12:38 AM
eduucaldas added inline comments.
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
118

True! The & default captures this already, but a better suggestion would be to use the capture: [S, this] or perhaps to just use S as an argument of the lambda.
That is a nitpick though, feel free to push the patch :)

gribozavr updated this revision to Diff 274010.Jun 29 2020, 1:56 AM

Simplified capture lists in lambdas

gribozavr2 marked an inline comment as done.Jun 29 2020, 2:05 AM
gribozavr2 added inline comments.
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
118

The & default captures this already

Oh indeed, thanks! I changed the capture list to [&] because I think it is not interesting what this closure captures exactly. It is not stored anywhere beyond the recordCallback call and invoked only once, so there are no lifetime concerns for variables captured by reference.

This revision was automatically updated to reflect the committed changes.

Is there a way this test case can somehow be broken up into multiple files? The test case takes a very long time to compile which causes intermittent but frequent failures on one of our bots that runs on a fairly small VM. Most of the failures listed here: http://lab.llvm.org:8011/builders/clang-ppc64le-linux?numbuilds=100 are timeout failures when compiling this test. It seems unfortunate to have to increase the timeout limit on the bot just for this - especially if this can be broken up into multiple files.

Hi @gribozavr do you think we can do something about this test?