Extract TextChildDumper class

[aaron.ballman]

include/clang/AST/ASTDumperUtils.h

23

I'm not certain this namespace is useful, especially when it gets imported at TU scope in ASTDumper.cpp.

97

I'm not sold on the name for this class. It's a bit too generic to understand what it does. How about ASTDumpLayoutFormatter (and ASTDumpNodeFormatter for the node dumper)?

98–111

I don't think these members should be public.

[steveire]

include/clang/AST/ASTDumperUtils.h

23

it gets imported at TU scope in ASTDumper.cpp

Today that's the only place it is used. In the future that won't be true.

[aaron.ballman]

include/clang/AST/ASTDumperUtils.h

23

Then we can add the namespace in the future when we find a situation where we're worried about collisions? As it stands, this only provides the illusion of safety. If you really want to keep it, please pull the global using declaration.

[steveire]

include/clang/AST/ASTDumperUtils.h

23

There is no need for such churn as adding the namespace later.

The using I added is beside two other usings in a cpp file. There is nothing 'global' about it.

97

I'll rename the class to ASTTextTreeStructure.

[steveire]

include/clang/AST/ASTDumperUtils.h

97

Or rather, TextTreeStructure.

[aaron.ballman]

include/clang/AST/ASTDumperUtils.h

23

The using I added is beside two other usings in a cpp file. There is nothing 'global' about it.

It applies to the entire TU, so it's global as far as this compilation unit is concerned. The existing "clang" and "llvm" using declarations are a pervasive thing we do but is not the general rule for how we use namespaces. See https://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std for more details.

I think the idea of an ast dumping namespace is worth exploring, but is orthogonal to this NFC patch. If you'd like to propose putting all AST dumping functionality into its own namespace, then please propose it as a separate patch so that we don't bog this one down with discussion that's not really germane to the rest of your patch. Questions I would love to see answered in that review are: why is a namespace needed, what should go into it (for instance, does ast printing go there, or just ast dumping, or all ast output utilities, etc), and whether it's planned to be used outside of its implementation file(s).

Alternatively, if you think this is germane to this patch, we can have that discussion here.

[steveire]

Rename class.

[steveire]

Remove namespace

[aaron.ballman]

LGTM aside from a commenting nit.

include/clang/AST/ASTDumperUtils.h

112

I really like the rename, but can you update this comment to not say "dump" as that implies to me that the writing happens immediately. It might also be a kindness to add the expected function signature of Fn to the comments.

include/clang/AST/ASTDumperUtils.h

+//===--- ASTDumperUtils.h - Printing of AST nodes -------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements AST utilities for traversal down the tree.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_ASTDUMPERUTILS_H
+#define LLVM_CLANG_AST_ASTDUMPERUTILS_H
+
+#include "clang/AST/ASTContext.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang {
+
+// Colors used for various parts of the AST dump
+// Do not use bold yellow for any text.  It is hard to read on white screens.
+
+struct TerminalColor {
+  raw_ostream::Colors Color;
+  bool Bold;
+};
+
+// Red           - CastColor
+// Green         - TypeColor
+// Bold Green    - DeclKindNameColor, UndeserializedColor
+// Yellow        - AddressColor, LocationColor
+// Blue          - CommentColor, NullColor, IndentColor
+// Bold Blue     - AttrColor
+// Bold Magenta  - StmtColor
+// Cyan          - ValueKindColor, ObjectKindColor
+// Bold Cyan     - ValueColor, DeclNameColor
+
+// Decl kind names (VarDecl, FunctionDecl, etc)
+static const TerminalColor DeclKindNameColor = {raw_ostream::GREEN, true};
+// Attr names (CleanupAttr, GuardedByAttr, etc)
+static const TerminalColor AttrColor = {raw_ostream::BLUE, true};
+// Statement names (DeclStmt, ImplicitCastExpr, etc)
+static const TerminalColor StmtColor = {raw_ostream::MAGENTA, true};
+// Comment names (FullComment, ParagraphComment, TextComment, etc)
+static const TerminalColor CommentColor = {raw_ostream::BLUE, false};
+
+// Type names (int, float, etc, plus user defined types)
+static const TerminalColor TypeColor = {raw_ostream::GREEN, false};
+
+// Pointer address
+static const TerminalColor AddressColor = {raw_ostream::YELLOW, false};
+// Source locations
+static const TerminalColor LocationColor = {raw_ostream::YELLOW, false};
+
+// lvalue/xvalue
+static const TerminalColor ValueKindColor = {raw_ostream::CYAN, false};
+// bitfield/objcproperty/objcsubscript/vectorcomponent
+static const TerminalColor ObjectKindColor = {raw_ostream::CYAN, false};
+
+// Null statements
+static const TerminalColor NullColor = {raw_ostream::BLUE, false};
+
+// Undeserialized entities
+static const TerminalColor UndeserializedColor = {raw_ostream::GREEN, true};
+
+// CastKind from CastExpr's
+static const TerminalColor CastColor = {raw_ostream::RED, false};
+
+// Value of the statement
+static const TerminalColor ValueColor = {raw_ostream::CYAN, true};
+// Decl names
+static const TerminalColor DeclNameColor = {raw_ostream::CYAN, true};
+
+// Indents ( `, -. | )
+static const TerminalColor IndentColor = {raw_ostream::BLUE, false};
+
+class ColorScope {
+  raw_ostream &OS;
+  const bool ShowColors;
+
+public:
+  ColorScope(raw_ostream &OS, bool ShowColors, TerminalColor Color)
+      : OS(OS), ShowColors(ShowColors) {
+    if (ShowColors)
+      OS.changeColor(Color.Color, Color.Bold);
+  }
+  ~ColorScope() {
+    if (ShowColors)
+      OS.resetColor();
+  }
+};
+
+class TextTreeStructure {
+  raw_ostream &OS;
+  const bool ShowColors;
+
+  /// Pending[i] is an action to dump an entity at level i.
+  llvm::SmallVector<std::function<void(bool isLastChild)>, 32> Pending;
+
+  /// Indicates whether we're at the top level.
+  bool TopLevel = true;
+
+  /// Indicates if we're handling the first child after entering a new depth.
+  bool FirstChild = true;
+
+  /// Prefix for currently-being-dumped entity.
+  std::string Prefix;
+
+public:
+  /// Dump a child of the current node.
+  template <typename Fn> void addChild(Fn doAddChild) {
+    // If we're at the top level, there's nothing interesting to do; just
+    // run the dumper.
+    if (TopLevel) {
+      TopLevel = false;
+      doAddChild();
+      while (!Pending.empty()) {
+        Pending.back()(true);
+        Pending.pop_back();
+      }
+      Prefix.clear();
+      OS << "\n";
+      TopLevel = true;
+      return;
+    }
+
+    auto dumpWithIndent = [this, doAddChild](bool isLastChild) {
+      // Print out the appropriate tree structure and work out the prefix for
+      // children of this node. For instance:
+      //
+      //   A        Prefix = ""
+      //   |-B      Prefix = "| "
+      //   | `-C    Prefix = "|   "
+      //   `-D      Prefix = "  "
+      //     |-E    Prefix = "  | "
+      //     `-F    Prefix = "    "
+      //   G        Prefix = ""
+      //
+      // Note that the first level gets no prefix.
+      {
+        OS << '\n';
+        ColorScope Color(OS, ShowColors, IndentColor);
+        OS << Prefix << (isLastChild ? '`' : '|') << '-';
+        this->Prefix.push_back(isLastChild ? ' ' : '|');
+        this->Prefix.push_back(' ');
+      }
+
+      FirstChild = true;
+      unsigned Depth = Pending.size();
+
+      doAddChild();
+
+      // If any children are left, they're the last at their nesting level.
+      // Dump those ones out now.
+      while (Depth < Pending.size()) {
+        Pending.back()(true);
+        this->Pending.pop_back();
+      }
+
+      // Restore the old prefix.
+      this->Prefix.resize(Prefix.size() - 2);
+    };
+
+    if (FirstChild) {
+      Pending.push_back(std::move(dumpWithIndent));
+    } else {
+      Pending.back()(false);
+      Pending.back() = std::move(dumpWithIndent);
+    }
+    FirstChild = false;
+  }
+
+  TextTreeStructure(raw_ostream &OS, bool ShowColors)
+      : OS(OS), ShowColors(ShowColors) {}
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_AST_ASTDUMPERUTILS_H

lib/AST/ASTDumper.cpp

 //===--- ASTDumper.cpp - Dumping implementation for ASTs ------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the AST dump methods, which dump out the
 // AST in a form that exposes type details and other fields.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTDumperUtils.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/CommentVisitor.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclLookups.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclOpenMP.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/LocInfoType.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace clang;
 using namespace clang::comments;
 
 //===----------------------------------------------------------------------===//
 // ASTDumper Visitor
 //===----------------------------------------------------------------------===//
 
 namespace  {
-  // Colors used for various parts of the AST dump
-  // Do not use bold yellow for any text.  It is hard to read on white screens.
-
-  struct TerminalColor {
-    raw_ostream::Colors Color;
-    bool Bold;
-  };
-
-  // Red           - CastColor
-  // Green         - TypeColor
-  // Bold Green    - DeclKindNameColor, UndeserializedColor
-  // Yellow        - AddressColor, LocationColor
-  // Blue          - CommentColor, NullColor, IndentColor
-  // Bold Blue     - AttrColor
-  // Bold Magenta  - StmtColor
-  // Cyan          - ValueKindColor, ObjectKindColor
-  // Bold Cyan     - ValueColor, DeclNameColor
-
-  // Decl kind names (VarDecl, FunctionDecl, etc)
-  static const TerminalColor DeclKindNameColor = { raw_ostream::GREEN, true };
-  // Attr names (CleanupAttr, GuardedByAttr, etc)
-  static const TerminalColor AttrColor = { raw_ostream::BLUE, true };
-  // Statement names (DeclStmt, ImplicitCastExpr, etc)
-  static const TerminalColor StmtColor = { raw_ostream::MAGENTA, true };
-  // Comment names (FullComment, ParagraphComment, TextComment, etc)
-  static const TerminalColor CommentColor = { raw_ostream::BLUE, false };
-
-  // Type names (int, float, etc, plus user defined types)
-  static const TerminalColor TypeColor = { raw_ostream::GREEN, false };
-
-  // Pointer address
-  static const TerminalColor AddressColor = { raw_ostream::YELLOW, false };
-  // Source locations
-  static const TerminalColor LocationColor = { raw_ostream::YELLOW, false };
-
-  // lvalue/xvalue
-  static const TerminalColor ValueKindColor = { raw_ostream::CYAN, false };
-  // bitfield/objcproperty/objcsubscript/vectorcomponent
-  static const TerminalColor ObjectKindColor = { raw_ostream::CYAN, false };
-
-  // Null statements
-  static const TerminalColor NullColor = { raw_ostream::BLUE, false };
-
-  // Undeserialized entities
-  static const TerminalColor UndeserializedColor = { raw_ostream::GREEN, true };
-
-  // CastKind from CastExpr's
-  static const TerminalColor CastColor = { raw_ostream::RED, false };
-
-  // Value of the statement
-  static const TerminalColor ValueColor = { raw_ostream::CYAN, true };
-  // Decl names
-  static const TerminalColor DeclNameColor = { raw_ostream::CYAN, true };
-
-  // Indents ( `, -. | )
-  static const TerminalColor IndentColor = { raw_ostream::BLUE, false };
-
-  class ColorScope {
-    raw_ostream &OS;
-    const bool ShowColors;
-
-  public:
-    ColorScope(raw_ostream &OS, bool ShowColors, TerminalColor Color)
-        : OS(OS), ShowColors(ShowColors) {
-      if (ShowColors)
-        OS.changeColor(Color.Color, Color.Bold);
-    }
-    ~ColorScope() {
-      if (ShowColors)
-        OS.resetColor();
-    }
-  };
 
   class ASTDumper
       : public ConstDeclVisitor<ASTDumper>,
         public ConstStmtVisitor<ASTDumper>,
         public ConstCommentVisitor<ASTDumper, void, const FullComment *>,
         public TypeVisitor<ASTDumper> {
 
+    TextTreeStructure TreeStructure;
+
     raw_ostream &OS;
     const CommandTraits *Traits;
     const SourceManager *SM;
 
     /// The policy to use for printing; can be defaulted.
     PrintingPolicy PrintPolicy;
 
-    /// Pending[i] is an action to dump an entity at level i.
-    llvm::SmallVector<std::function<void(bool isLastChild)>, 32> Pending;
-
     /// Indicates whether we should trigger deserialization of nodes that had
     /// not already been loaded.
     bool Deserialize = false;
 
-    /// Indicates whether we're at the top level.
-    bool TopLevel = true;
-
-    /// Indicates if we're handling the first child after entering a new depth.
-    bool FirstChild = true;
-
-    /// Prefix for currently-being-dumped entity.
-    std::string Prefix;
-
     /// Keep track of the last location we print out so that we can
     /// print out deltas from then on out.
     const char *LastLocFilename = "";
     unsigned LastLocLine = ~0U;
 
     const bool ShowColors;
 
     /// Dump a child of the current node.
     template<typename Fn> void dumpChild(Fn doDumpChild) {
-      // If we're at the top level, there's nothing interesting to do; just
-      // run the dumper.
-      if (TopLevel) {
-        TopLevel = false;
-        doDumpChild();
-        while (!Pending.empty()) {
-          Pending.back()(true);
-          Pending.pop_back();
-        }
-        Prefix.clear();
-        OS << "\n";
-        TopLevel = true;
-        return;
-      }
-
-      auto dumpWithIndent = [this, doDumpChild](bool isLastChild) {
-        // Print out the appropriate tree structure and work out the prefix for
-        // children of this node. For instance:
-        //
-        //   A        Prefix = ""
-        //   |-B      Prefix = "| "
-        //   | `-C    Prefix = "|   "
-        //   `-D      Prefix = "  "
-        //     |-E    Prefix = "  | "
-        //     `-F    Prefix = "    "
-        //   G        Prefix = ""
-        //
-        // Note that the first level gets no prefix.
-        {
-          OS << '\n';
-          ColorScope Color(OS, ShowColors, IndentColor);
-          OS << Prefix << (isLastChild ? '`' : '|') << '-';
-          this->Prefix.push_back(isLastChild ? ' ' : '|');
-          this->Prefix.push_back(' ');
-        }
-
-        FirstChild = true;
-        unsigned Depth = Pending.size();
-
-        doDumpChild();
-
-        // If any children are left, they're the last at their nesting level.
-        // Dump those ones out now.
-        while (Depth < Pending.size()) {
-          Pending.back()(true);
-          this->Pending.pop_back();
-        }
-
-        // Restore the old prefix.
-        this->Prefix.resize(Prefix.size() - 2);
-      };
-
-      if (FirstChild) {
-        Pending.push_back(std::move(dumpWithIndent));
-      } else {
-        Pending.back()(false);
-        Pending.back() = std::move(dumpWithIndent);
-      }
-      FirstChild = false;
+      TreeStructure.addChild(doDumpChild);
     }
 
   public:
     ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
               const SourceManager *SM)
         : ASTDumper(OS, Traits, SM,
                     SM && SM->getDiagnostics().getShowColors()) {}
 
     ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
               const SourceManager *SM, bool ShowColors)
         : ASTDumper(OS, Traits, SM, ShowColors, LangOptions()) {}
     ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
               const SourceManager *SM, bool ShowColors,
               const PrintingPolicy &PrintPolicy)
-        : OS(OS), Traits(Traits), SM(SM), PrintPolicy(PrintPolicy),
-          ShowColors(ShowColors) {}
+        : TreeStructure(OS, ShowColors), OS(OS), Traits(Traits), SM(SM),
+          PrintPolicy(PrintPolicy), ShowColors(ShowColors) {}
 
     void setDeserialize(bool D) { Deserialize = D; }
 
     void dumpDecl(const Decl *D);
     void dumpStmt(const Stmt *S);
     void dumpFullComment(const FullComment *C);
 
     // Utilities