aaron.ballman added inline comments.
================
Comment at: include/clang/AST/TextNodeDumper.h:28
const comments::FullComment *> {
+ TextTreeStructure &TreeStructure;
raw_ostream &OS;
----------------
steveire wrote:
> steveire wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > Actually 'the stars would have to line up in a particular way' in order
> > > to reach the scenario you are concerned about. What would have to happen
> > > is:
> > >
> > > * This patch gets in as-is
> > > * Someone in the future reorders the members
> > > * But they don't reorder the init-list
> > > * They build on a platform without -Wreorder (only MSVC?) enabled in the
> > > build.
> > > * That passes review
> > > * Other users update their checkout and everyone else also ignores the
> > > -Wreorder warning.
> > >
> > > That is a vanishingly likely scenario. It's just unreasonable to consider
> > > that as a reason to create a broken interface.
> > >
> > > And it would be a broken interface.
> > >
> > > After the refactoring is complete, we have something like
> > >
> > > ```
> > > class ASTDumper
> > > : public ASTDumpTraverser<ASTDumper, TextTreeStructure,
> > > TextNodeDumper> {
> > > TextTreeStructure TreeStructure;
> > > TextNodeDumper NodeDumper;
> > > public:
> > > TextTreeStructure &getTreeStructure() { return TreeStructure; }
> > > TextNodeDumper &getNodeDumper() { return NodeDumper; }
> > >
> > > ASTDumper(raw_ostream &OS, const SourceManager *SM)
> > > : TreeStructure(OS),
> > > NodeDumper(TreeStructure, OS, SM) {}
> > > };
> > >
> > > ```
> > >
> > > In the case, of the `ASTDumper`, the `TextNodeDumper` uses the
> > > `TextTreeStructure`.
> > >
> > > However, in the case of any other subclass of `ASTDumpTraverser`, the
> > > `NodeDumper` type does not depend on the `TreeStructure` type. The
> > > `ASTDumpTraverser` should not pass the `TreeStructure` to the
> > > `NodeDumper`because the `ASTDumpTraverser` should not assume the
> > > `NodeDumper` needs the `ASTDumpTraverser`.
> > >
> > > That is true in one concrete case (the `TextNodeDumper`), but not in
> > > general. You would be encoding an assumption about a concrete
> > > `NodeDumper` implementation in the generic `ASTDumpTraverser`.
> > >
> > > That is an interface violation which is definitely not justified by your
> > > far-fetched scenario of someone re-ordering the members in the future and
> > > ignoring `-Wreorder`.
> > Should be "should not assume the `NodeDumper` needs the `TreeStructure`",
> > sorry.
> I believe if something like https://reviews.llvm.org/D56407 is accepted, then
>
> * The generic traverser will not artificially couple the `TreeStructure` and
> the `NodeVisitor`
> * The end-result ASTDumper will not have two members with
> reference-relationship and there will be no ordering issue.
>
I agree; I think that's a good way to solve this problem.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55337/new/
https://reviews.llvm.org/D55337
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
