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