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


Repository:
  rC Clang

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

https://reviews.llvm.org/D55337