Page MenuHomePhabricator

Add a Visit overload for DynTypedNode to ASTNodeTraverser
ClosedPublic

Authored by steveire on May 12 2019, 1:14 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

steveire created this revision.May 12 2019, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 1:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

What will be making use of/testing this new functionality?

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

Do we currently have any such code that's using this functionality, though? I'm mostly concerned that this is dead code with no testing, currently. The functionality itself seems reasonable enough and the code looks correct enough, so if this is part of a series of planned changes, that'd be good information to have for the review.

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

Do we currently have any such code that's using this functionality, though? I'm mostly concerned that this is dead code with no testing, currently. The functionality itself seems reasonable enough and the code looks correct enough, so if this is part of a series of planned changes, that'd be good information to have for the review.

Ah, yes. This is supposed to be 'useful public API' like the other Visit methods for use inside and outside the codebase. A follow-up patch will use it, but it's provided for external use too anyway.

I'll add a unit test.

steveire updated this revision to Diff 199922.May 16 2019, 3:54 PM

Add basic traverser test

aaron.ballman accepted this revision.May 17 2019, 6:34 AM

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

Do we currently have any such code that's using this functionality, though? I'm mostly concerned that this is dead code with no testing, currently. The functionality itself seems reasonable enough and the code looks correct enough, so if this is part of a series of planned changes, that'd be good information to have for the review.

Ah, yes. This is supposed to be 'useful public API' like the other Visit methods for use inside and outside the codebase. A follow-up patch will use it, but it's provided for external use too anyway.

I'll add a unit test.

Ahh, thank you! It makes a lot more sense to me now. LGTM aside from some nits.

include/clang/AST/ASTNodeTraverser.h
208 ↗(On Diff #199992)

Can you add a comment here: // FIXME: Improve this with a switch or a visitor pattern. (We have a similar comment in similar-looking code in ASTMatchFinder.cpp:476.)

unittests/AST/ASTTraverserTest.cpp
75 ↗(On Diff #199992)

Did clang-format produce this formatting? (If not, run through clang-format.)

89 ↗(On Diff #199992)

Name instead

97 ↗(On Diff #199992)

DumpString -- same elsewhere.

This revision is now accepted and ready to land.May 17 2019, 6:34 AM
steveire marked 5 inline comments as done.May 17 2019, 6:53 AM
steveire added inline comments.
unittests/AST/ASTTraverserTest.cpp
75 ↗(On Diff #199992)

Yep, clang-format made it like this.

This revision was automatically updated to reflect the committed changes.
steveire marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 6:53 AM
aaron.ballman added inline comments.May 17 2019, 6:57 AM
unittests/AST/ASTTraverserTest.cpp
75 ↗(On Diff #199992)

Weird that it added the extra whitespace -- I think this is the only instance where we have whitespace to either side of the adornments, which is what caught me by surprise.