[PATCH] D61834: Add a Visit overload for DynTypedNode to ASTNodeTraverser

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D61834#1505418 <https://reviews.llvm.org/D61834#1505418>, @steveire wrote:

> In D61834#1505124 <https://reviews.llvm.org/D61834#1505124>, @aaron.ballman wrote:
>
> > In D61834#1505056 <https://reviews.llvm.org/D61834#1505056>, @steveire wrote:
> >
> > > In D61834#1504665 <https://reviews.llvm.org/D61834#1504665>, @aaron.ballman wrote:
> > >
> > > > 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.



================
Comment at: include/clang/AST/ASTNodeTraverser.h:208
 
+  void Visit(const ast_type_traits::DynTypedNode &N) {
+    if (const auto *D = N.get<Decl>())
----------------
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.)


================
Comment at: unittests/AST/ASTTraverserTest.cpp:75
+
+template <typename... NodeType> std::string dumpASTString(NodeType &&... N) {
+  std::string Buffer;
----------------
Did clang-format produce this formatting? (If not, run through clang-format.)


================
Comment at: unittests/AST/ASTTraverserTest.cpp:89
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+                                    const std::string &name) {
+  auto Result = ast_matchers::match(functionDecl(hasName(name)).bind("fn"),
----------------
`Name` instead


================
Comment at: unittests/AST/ASTTraverserTest.cpp:97
+template <typename T> struct Verifier {
+  static void withDynNode(T Node, const std::string &dumpString) {
+    EXPECT_EQ(dumpASTString(ast_type_traits::DynTypedNode::create(Node)),
----------------
`DumpString` -- same elsewhere.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61834