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