[PATCH] D55337: NFC: Move dumpDeclRef to NodeDumper

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked an inline comment as done.
steveire added inline comments.


================
Comment at: include/clang/AST/TextNodeDumper.h:28
                                            const comments::FullComment *> {
+  TextTreeStructure &TreeStructure;
   raw_ostream &OS;
----------------
aaron.ballman wrote:
> 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.
In your first snippet there is a dangling reference because the author of `MySuperAwesomeASTDumper` decided to make the members references. If the members are references, code like your first snippet will cause dangling references and nothing can prevent that. Adding `TreeStructure&` to Visit methods as you suggested does not prevent it.

The only solution is make the `MySuperAwesomeASTDumper` not use member references (ie your second snippet). The order is then in fact not problematic because "taking a reference to an uninitialized object is legal".


Repository:
  rC Clang

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

https://reviews.llvm.org/D55337