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

label -> Label
doAddChild -> DoAddChild

117

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

lib/AST/ASTDumper.cpp

67

Label, DoDumpChild

89

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.

1717–1718

label -> Label

[steveire]

Update with new approach

[steveire]

Clean up API a bit.

[steveire]

lib/AST/ASTDumper.cpp

89

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

[aaron.ballman]

lib/AST/ASTDumper.cpp

89

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 ↗

(On Diff #180754)

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 ↗

(On Diff #180754)

doAddChild -> DoAddChild

48 ↗

(On Diff #180754)

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

51 ↗

(On Diff #180754)

doAddChild -> DoAddChild

68 ↗

(On Diff #180754)

deffered -> deferred

70 ↗

(On Diff #180754)

dumpWithIndent -> DumpWithIndent
isLastChild -> IsLastChild

87 ↗

(On Diff #180754)

Elide braces

[steveire]

Nits

[aaron.ballman]

Aside from cosmetic nits, this LGTM.

include/clang/AST/ASTDumperUtils.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 with a label.
+  /// Calls doAddChild without arguments
+  template <typename Fn> void addChild(const char *label, Fn doAddChild) {
+    if (!label)
+      return addChild(doAddChild);
+    addChild([=] {
+      OS << label;
+      addChild(doAddChild);
+    });
+  }
   /// Add a child of the current node.  Calls doAddChild without arguments
   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()) {

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) {
       TreeStructure.addChild(doDumpChild);
     }
+    template <typename Fn> void dumpChild(const char *label, Fn doDumpChild) {
+      TreeStructure.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)
         : TreeStructure(OS, ShowColors),
           NodeDumper(OS, ShowColors, SM, PrintPolicy), OS(OS), Traits(Traits),
           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, const char *label = nullptr);
 
     // Utilities
     void dumpType(QualType T) { NodeDumper.dumpType(T); }
     void dumpTypeAsChild(QualType T);
     void dumpTypeAsChild(const Type *T);
     void dumpDeclRef(const Decl *Node, const char *Label = nullptr);
     void dumpBareDeclRef(const Decl *Node) { NodeDumper.dumpBareDeclRef(Node); }
     void dumpDeclContext(const DeclContext *DC);
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, const char *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";