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