Page MenuHomePhabricator

NFC: Move dumpDeclRef to NodeDumper
ClosedPublic

Authored by steveire on Dec 5 2018, 1:07 PM.

Diff Detail

Repository
rC Clang

Event Timeline

steveire created this revision.Dec 5 2018, 1:07 PM
aaron.ballman added inline comments.Dec 6 2018, 5:21 AM
include/clang/AST/TextNodeDumper.h
110

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.

steveire marked an inline comment as done.Dec 6 2018, 9:48 AM
steveire added inline comments.
include/clang/AST/TextNodeDumper.h
110

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".

aaron.ballman added inline comments.Jan 4 2019, 8:33 AM
include/clang/AST/TextNodeDumper.h
110
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.

steveire marked an inline comment as done.Jan 5 2019, 10:23 AM
steveire added inline comments.
include/clang/AST/TextNodeDumper.h
110

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 NodeDumperbecause 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.

steveire marked an inline comment as done.Jan 5 2019, 10:24 AM
steveire added inline comments.
include/clang/AST/TextNodeDumper.h
110

Should be "should not assume the NodeDumper needs the TreeStructure", sorry.

steveire marked an inline comment as done.Jan 7 2019, 1:18 PM
steveire added inline comments.
include/clang/AST/TextNodeDumper.h
110

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.
aaron.ballman added inline comments.Jan 8 2019, 8:29 AM
include/clang/AST/TextNodeDumper.h
110

I agree; I think that's a good way to solve this problem.

aaron.ballman accepted this revision.Jan 8 2019, 10:30 AM

Once D56407 lands, the rebased changes here LGTM

This revision is now accepted and ready to land.Jan 8 2019, 10:30 AM
This revision was automatically updated to reflect the committed changes.