Page MenuHomePhabricator

NFC: Move dumpDeclRef to NodeDumper
Needs ReviewPublic

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

Details

Reviewers
aaron.ballman

Diff Detail

Event Timeline

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

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.Thu, Dec 6, 9:48 AM
steveire added inline comments.
include/clang/AST/TextNodeDumper.h
28

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