aaron.ballman added inline comments.
================
Comment at: include/clang/AST/TextNodeDumper.h:28
const comments::FullComment *> {
+ TextTreeStructure &TreeStructure;
raw_ostream &OS;
----------------
steveire wrote:
> 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".
> The order is then in fact not problematic because "taking a reference to an
> uninitialized object is legal".
This presumes that the constructors aren't using those references to the
uninitialized object, which would be illegal. That's what I mean about this
being very fragile -- if the stars line up correctly, everything works fine,
but if the stars aren't aligned just right, you get really hard problems to
track down.
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
