Add a Visit overload for DynTypedNode to ASTNodeTraverser

[steveire]

Format

[aaron.ballman]

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

[steveire]

In 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.

[aaron.ballman]

In D61834#1505056, @steveire wrote:

In 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.

[steveire]

In D61834#1505124, @aaron.ballman wrote:

In D61834#1505056, @steveire wrote:

In 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.

[steveire]

Add basic traverser test

[steveire]

Update

[aaron.ballman]

In D61834#1505418, @steveire wrote:

In D61834#1505124, @aaron.ballman wrote:

In D61834#1505056, @steveire wrote:

In 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.

include/clang/AST/ASTNodeTraverser.h

208

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

76

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

90

Name instead

98

DumpString -- same elsewhere.

[steveire]

unittests/AST/ASTTraverserTest.cpp

76

Yep, clang-format made it like this.

[aaron.ballman]

unittests/AST/ASTTraverserTest.cpp

76

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.

include/clang/AST/ASTNodeTraverser.h

getNodeDelegate().AddChild([=] {
                                                                           FC);
       for (comments::Comment::child_iterator I = C->child_begin(),
                                              E = C->child_end();
            I != E; ++I)
         Visit(*I, FC);
     });
   }
 
+  void Visit(const ast_type_traits::DynTypedNode &N) {
+    if (const auto *D = N.get<Decl>())
+      Visit(D);
+    else if (const auto *S = N.get<Stmt>())
+      Visit(S);
+    else if (const auto *QT = N.get<QualType>())
+      Visit(*QT);
+    else if (const auto *T = N.get<Type>())
+      Visit(T);
+    else if (const auto *A = N.get<Attr>())
+      Visit(A);
+    else if (const auto *C = N.get<CXXCtorInitializer>())
+      Visit(C);
+    else if (const auto *C = N.get<OMPClause>())
+      Visit(C);
+    else if (const auto *C = N.get<comments::FullComment>())
+      Visit(C, C);
+    else if (const auto *T = N.get<TemplateArgument>())
+      Visit(*T);
+  }
+
   void dumpDeclContext(const DeclContext *DC) {
     if (!DC)
       return;
 
     for (const auto *D : (Deserialize ? DC->decls() : DC->noload_decls()))
       Visit(D);
   }
 

unittests/AST/ASTTraverserTest.cpp

+//===- unittests/AST/ASTTraverserTest.h------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTNodeTraverser.h"
+#include "clang/AST/TextNodeDumper.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace clang::tooling;
+using namespace clang::ast_matchers;
+
+namespace clang {
+
+class NodeTreePrinter : public TextTreeStructure {
+  llvm::raw_ostream &OS;
+
+public:
+  NodeTreePrinter(llvm::raw_ostream &OS)
+      : TextTreeStructure(OS, /* showColors */ false), OS(OS) {}
+
+  void Visit(const Decl *D) { OS << D->getDeclKindName() << "Decl"; }
+
+  void Visit(const Stmt *S) { OS << S->getStmtClassName(); }
+
+  void Visit(QualType QT) {
+    OS << "QualType " << QT.split().Quals.getAsString();
+  }
+
+  void Visit(const Type *T) { OS << T->getTypeClassName() << "Type"; }
+
+  void Visit(const comments::Comment *C, const comments::FullComment *FC) {
+    OS << C->getCommentKindName();
+  }
+
+  void Visit(const CXXCtorInitializer *Init) { OS << "CXXCtorInitializer"; }
+
+  void Visit(const Attr *A) {
+    switch (A->getKind()) {
+#define ATTR(X)                                                                \
+  case attr::X:                                                                \
+    OS << #X;                                                                  \
+    break;
+#include "clang/Basic/AttrList.inc"
+    }
+    OS << "Attr";
+  }
+
+  void Visit(const OMPClause *C) { OS << "OMPClause"; }
+  void Visit(const TemplateArgument &A, SourceRange R = {},
+             const Decl *From = nullptr, const char *Label = nullptr) {
+    OS << "TemplateArgument";
+  }
+
+  template <typename... T> void Visit(T...) {}
+};
+
+class TestASTDumper : public ASTNodeTraverser<TestASTDumper, NodeTreePrinter> {
+
+  NodeTreePrinter MyNodeRecorder;
+
+public:
+  TestASTDumper(llvm::raw_ostream &OS) : MyNodeRecorder(OS) {}
+  NodeTreePrinter &doGetNodeDelegate() { return MyNodeRecorder; }
+};
+
+template <typename... NodeType> std::string dumpASTString(NodeType &&... N) {
+  std::string Buffer;
+  llvm::raw_string_ostream OS(Buffer);
+
+  TestASTDumper Dumper(OS);
+
+  OS << "\n";
+
+  Dumper.Visit(std::forward<NodeType &&>(N)...);
+
+  return OS.str();
+}
+
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+                                    const std::string &name) {
+  auto Result = ast_matchers::match(functionDecl(hasName(name)).bind("fn"),
+                                    AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs<FunctionDecl>("fn");
+}
+
+template <typename T> struct Verifier {
+  static void withDynNode(T Node, const std::string &dumpString) {
+    EXPECT_EQ(dumpASTString(ast_type_traits::DynTypedNode::create(Node)),
+              dumpString);
+  }
+};
+
+template <typename T> struct Verifier<T *> {
+  static void withDynNode(T *Node, const std::string &dumpString) {
+    EXPECT_EQ(dumpASTString(ast_type_traits::DynTypedNode::create(*Node)),
+              dumpString);
+  }
+};
+
+template <typename T>
+void verifyWithDynNode(T Node, const std::string &dumpString) {
+  EXPECT_EQ(dumpASTString(Node), dumpString);
+
+  Verifier<T>::withDynNode(Node, dumpString);
+}
+
+TEST(Traverse, Dump) {
+
+  auto AST = buildASTFromCode(R"cpp(
+struct A {
+  int m_number;
+
+  /// CTor
+  A() : m_number(42) {}
+
+  [[nodiscard]] const int func() {
+    return 42;
+  }
+
+};
+
+template<typename T>
+struct templ
+{ 
+};
+
+template<>
+struct templ<int>
+{ 
+};
+
+)cpp");
+
+  const FunctionDecl *Func = getFunctionNode(AST.get(), "func");
+
+  verifyWithDynNode(Func,
+                    R"cpp(
+CXXMethodDecl
+|-CompoundStmt
+| `-ReturnStmt
+|   `-IntegerLiteral
+`-WarnUnusedResultAttr
+)cpp");
+
+  Stmt *Body = Func->getBody();
+
+  verifyWithDynNode(Body,
+                    R"cpp(
+CompoundStmt
+`-ReturnStmt
+  `-IntegerLiteral
+)cpp");
+
+  QualType QT = Func->getType();
+
+  verifyWithDynNode(QT,
+                    R"cpp(
+FunctionProtoType
+`-QualType const
+  `-BuiltinType
+)cpp");
+
+  const FunctionDecl *CTorFunc = getFunctionNode(AST.get(), "A");
+
+  verifyWithDynNode(CTorFunc->getType(),
+                    R"cpp(
+FunctionProtoType
+`-BuiltinType
+)cpp");
+
+  Attr *A = *Func->attr_begin();
+
+  EXPECT_EQ(dumpASTString(A),
+            R"cpp(
+WarnUnusedResultAttr
+)cpp");
+
+  auto *CTor = dyn_cast<CXXConstructorDecl>(CTorFunc);
+  const CXXCtorInitializer *Init = *CTor->init_begin();
+
+  verifyWithDynNode(Init,
+                    R"cpp(
+CXXCtorInitializer
+`-IntegerLiteral
+)cpp");
+
+  const comments::FullComment *Comment =
+      AST->getASTContext().getLocalCommentForDeclUncached(CTorFunc);
+
+  EXPECT_EQ(dumpASTString(Comment, Comment),
+            R"cpp(
+FullComment
+`-ParagraphComment
+  `-TextComment
+)cpp");
+
+  auto Result = ast_matchers::match(
+      classTemplateSpecializationDecl(hasName("templ")).bind("fn"),
+      AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  auto Templ = Result[0].getNodeAs<ClassTemplateSpecializationDecl>("fn");
+
+  TemplateArgument TA = Templ->getTemplateArgs()[0];
+
+  verifyWithDynNode(TA,
+                    R"cpp(
+TemplateArgument
+)cpp");
+}
+} // namespace clang

unittests/AST/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 if (MSVC)
   set_source_files_properties(ASTImporterTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterGenericRedeclTest.cpp
   ASTImporterVisibilityTest.cpp
+  ASTTraverserTest.cpp
   ASTTypeTraitsTest.cpp
   ASTVectorTest.cpp
   CommentLexer.cpp
   CommentParser.cpp
   CommentTextTest.cpp
   DataCollectionTest.cpp
   DeclPrinterTest.cpp
   DeclTest.cpp