[PATCH] D55337: NFC: Move dumpDeclRef to NodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/TextNodeDumper.h:28
                                            const comments::FullComment *> {
+  TextTreeStructure &TreeStructure;
   raw_ostream &OS;
----------------
This makes me a bit wary because you create a node dumper in the same situations you make a tree structure object, but now there's a strict ordering between the two object creations. If you're doing this construction local to a function, you wind up with a dangling reference unless you're careful (which is unfortunate, but not the end of the world). If you're doing this construction as part of a constructor's initializer list, you now have to properly order the member declarations within the class and that is also unfortunate. Given that those are the two common scenarios for how I envision constructing an ast dump of some kind, I worry about the fragility. e.g.,
```
unique_ptr<ASTConsumer> createASTDumper(...) {
  TextTreeStructure TreeStructure;
  TextNodeDumper NodeDumper(TreeStructure); // Oops, dangling reference
  return make_unique<MySuperAwesomeASTDumper>(TreeStructure, NodeDumper, ...);
}

// vs

struct MySuperAwesomeASTDumper : ... {
  MySuperAwesomeASTDumper() : TreeStructure(...), NodeDumper(TreeStructure, ...) {}
private:
  TextTreeStructure TreeStructure; // This order is now SUPER important
  TextNodeDumper NodeDumper;
};
```
There's a part of me that wonders if a better approach is to have this object passed to the `dumpFoo()` calls as a reference parameter. This way, the caller is still responsible for creating an object, but the creation order between the tree and the node dumper isn't as fragile.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55337