Details
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 25921 Build 25920: arc lint + arc unit
Event Timeline
| 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. | |
| 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". | |
| include/clang/AST/TextNodeDumper.h | ||
|---|---|---|
| 28 |
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. | |
| include/clang/AST/TextNodeDumper.h | ||
|---|---|---|
| 28 | 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:
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. | |
| include/clang/AST/TextNodeDumper.h | ||
|---|---|---|
| 28 | Should be "should not assume the NodeDumper needs the TreeStructure", sorry. | |
| include/clang/AST/TextNodeDumper.h | ||
|---|---|---|
| 28 | I believe if something like https://reviews.llvm.org/D56407 is accepted, then
| |
| include/clang/AST/TextNodeDumper.h | ||
|---|---|---|
| 28 | I agree; I think that's a good way to solve this problem. | |
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.