[PATCH] D55488: Add utility for dumping a label with child nodes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/ASTDumperUtils.h:115
+  template <typename Fn>
+  void addChild(const std::string &label, Fn doAddChild) {
+    if (label.empty())
----------------
label -> Label
doAddChild -> DoAddChild


================
Comment at: include/clang/AST/ASTDumperUtils.h:117
+    if (label.empty())
+      return addChild(doAddChild);
+    addChild([=] {
----------------
While correct, this is a bit too clever -- can you switch to using a more idiomatic return statement?


================
Comment at: lib/AST/ASTDumper.cpp:66
+    template <typename Fn>
+    void dumpChild(const std::string &label, Fn doDumpChild) {
+      TreeStructure.addChild(label, doDumpChild);
----------------
Label, DoDumpChild


================
Comment at: lib/AST/ASTDumper.cpp:89
     void dumpDecl(const Decl *D);
-    void dumpStmt(const Stmt *S);
+    void dumpStmt(const Stmt *S, const std::string &label = {});
 
----------------
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.


================
Comment at: lib/AST/ASTDumper.cpp:1692
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, const std::string &label) {
+  dumpChild(label, [=] {
----------------
label -> Label


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55488/new/

https://reviews.llvm.org/D55488