[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;
----------------
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.



Repository:
  rC Clang

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

https://reviews.llvm.org/D55337