Add utility for dumping a label with child nodes

[steveire]

Summary

Use it to add optional label nodes to Stmt dumps. This preserves
behavior of InitExprList dump:

CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]'
CHECK-NEXT: |-array filler: InitListExpr {{.+}} <col:15> 'U' field Field {{.+}} 'i' 'int'
CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U' field Field {{.+}} 'i' 'int'
CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14> 'int' 1

[steveire]

Use std::string

[aaron.ballman]

include/clang/AST/ASTDumperUtils.h

115 ↗

(On Diff #177419)

label -> Label
doAddChild -> DoAddChild

117 ↗

(On Diff #177419)

While correct, this is a bit too clever -- can you switch to using a more idiomatic return statement?

lib/AST/ASTDumper.cpp

65

Label, DoDumpChild

86

Label

Rather than using {}, how about "" (same behavior, but looks more idiomatic).

Why std::string instead of StringRef? I expect this will be called mostly with string literals, which saves an allocation. The other labels are using const char *, which would be another reasonable option. Whatever we go with, it'd be nice to make the label types agree across the calls.

1691–1692

label -> Label

[steveire]

Update with new approach

[steveire]

Clean up API a bit.

[steveire]

lib/AST/ASTDumper.cpp

86

The actual print in TextTreeStructure is deferred, so it can't be const char* or StringRef.

[aaron.ballman]

lib/AST/ASTDumper.cpp

86

I think I've convinced myself there are not lifetime issues here, but it's subtle. In the case where the default argument is used, it creates a temporary object that is lifetime extended for the duration of the full expression including the call to dumpStmt(). The capture of Label in the lambda in addChild() captures the reference by copy and initializes the implicit field for the capture to the temporary value while its still within its lifetime. So I don't think this introduces UB.

However, if that lambda ever changes the capture list to use capture defaults (which we typically prefer when capturing multiple entities) and someone uses & or this as the capture default, they're going to have a very hard-to-discover bug on their hands. :-/

Another approach is to use StringRef in these calls, but within addChild() change the lambda to capture a local std::string by copy. e.g.,

template <typename Fn>
void addChild(StringRef Label, Fn doAddChild) {
  ...
  std::string LabelStr = Label.str();
  auto dumpWithIndent = [this, doAddChild, LabelStr](bool isLastChild) {
    ...
  };
  ...

(And when we upgrade to C++14 we can drop the local and just use an init capture instead.)

[steveire]

Replace std::string with StringRef

[aaron.ballman]

@rsmith -- do you have opinions on the new format for how we display the child node? I think this is a more clear approach, but a second opinion would be nice.

include/clang/AST/TextNodeDumper.h

43

Why move this comment down a line? Might as well leave it where it was before (feel free to add a full stop to the end of the comment though).

45

doAddChild -> DoAddChild

48

Can you add a newline above this comment to give it some visual space?

51

doAddChild -> DoAddChild

66

deffered -> deferred

68

dumpWithIndent -> DumpWithIndent
isLastChild -> IsLastChild

84

Elide braces

[steveire]

Nits

[aaron.ballman]

Aside from cosmetic nits, this LGTM.

include/clang/AST/TextNodeDumper.h

class TextTreeStructure {
   /// 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:
   /// Add a child of the current node.  Calls DoAddChild without arguments
   template <typename Fn> void AddChild(Fn DoAddChild) {
+    return AddChild("", DoAddChild);
+  }
+
+  /// Add a child of the current node with an optional label.
+  /// Calls DoAddChild without arguments.
+  template <typename Fn> void AddChild(StringRef Label, 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) {
+    // We need to capture an owning-string in the lambda because the lambda
+    // is invoked in a deferred manner.
+    std::string LabelStr = Label;
+    auto DumpWithIndent = [this, DoAddChild, LabelStr](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 ? '`' : '|') << '-';
+        if (!LabelStr.empty())
+          OS << LabelStr << ": ";
+
         this->Prefix.push_back(IsLastChild ? ' ' : '|');
         this->Prefix.push_back(' ');
       }
 
       FirstChild = true;
       unsigned Depth = Pending.size();
 
       DoAddChild();

lib/AST/ASTDumper.cpp

class ASTDumper
     bool Deserialize = false;
 
     const bool ShowColors;
 
     /// Dump a child of the current node.
     template<typename Fn> void dumpChild(Fn DoDumpChild) {
       NodeDumper.AddChild(DoDumpChild);
     }
+    template <typename Fn> void dumpChild(StringRef Label, Fn DoDumpChild) {
+      NodeDumper.AddChild(Label, 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)
         : NodeDumper(OS, ShowColors, SM, PrintPolicy, Traits), OS(OS),
           PrintPolicy(PrintPolicy), ShowColors(ShowColors) {}
 
     void setDeserialize(bool D) { Deserialize = D; }
 
     void dumpDecl(const Decl *D);
-    void dumpStmt(const Stmt *S);
+    void dumpStmt(const Stmt *S, StringRef Label = {});
 
     // Utilities
     void dumpType(QualType T) { NodeDumper.dumpType(T); }
     void dumpTypeAsChild(QualType T);
     void dumpTypeAsChild(const Type *T);
     void dumpBareDeclRef(const Decl *Node) { NodeDumper.dumpBareDeclRef(Node); }
     void dumpDeclContext(const DeclContext *DC);
     void dumpLookups(const DeclContext *DC, bool DumpDecls);
void ASTDumper::VisitBlockDecl(const BlockDecl *D) {
   }
   dumpStmt(D->getBody());
 }
 
 //===----------------------------------------------------------------------===//
 //  Stmt dumping methods.
 //===----------------------------------------------------------------------===//
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, StringRef Label) {
+  dumpChild(Label, [=] {
     if (!S) {
       ColorScope Color(OS, ShowColors, NullColor);
       OS << "<<<NULL>>>";
       return;
     }
     {
       ColorScope Color(OS, ShowColors, StmtColor);
       OS << S->getStmtClassName();

 }
 
 void ASTDumper::VisitInitListExpr(const InitListExpr *ILE) {
   if (auto *Field = ILE->getInitializedFieldInUnion()) {
     OS << " field ";
     NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-    dumpChild([=] {
-      OS << "array filler";
-      dumpStmt(Filler);
-    });
+    dumpStmt(Filler, "array_filler");
   }
 }
 
 void ASTDumper::VisitUnaryOperator(const UnaryOperator *Node) {
   OS << " " << (Node->isPostfix() ? "postfix" : "prefix")
      << " '" << UnaryOperator::getOpcodeStr(Node->getOpcode()) << "'";
   if (!Node->canOverflow())
     OS << " cannot overflow";

test/AST/ast-dump-stmt.cpp

union U {
   long l;
 };
 
 void TestUnionInitList()
 {
   U us[3] = {1};
 // CHECK: VarDecl {{.+}} <col:3, col:15> col:5 us 'U [3]' cinit
 // CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]'
-// CHECK-NEXT:   |-array filler
-// CHECK-NEXT:   | `-InitListExpr {{.+}} <col:15> 'U' field Field {{.+}} 'i' 'int'
+// CHECK-NEXT:   |-array_filler: InitListExpr {{.+}} <col:15> 'U' field Field {{.+}} 'i' 'int'
 // CHECK-NEXT:   `-InitListExpr {{.+}} <col:14> 'U' field Field {{.+}} 'i' 'int'
 // CHECK-NEXT:     `-IntegerLiteral {{.+}} <col:14> 'int' 1
 }
 
 void TestSwitch(int i) {
   switch (int a; i)
     ;
   // CHECK: SwitchStmt 0x{{[^ ]*}} <line:[[@LINE-2]]:3, line:[[@LINE-1]]:5> has_init