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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
