aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D61835#1505388 <https://reviews.llvm.org/D61835#1505388>, @steveire wrote: > In D61835#1505314 <https://reviews.llvm.org/D61835#1505314>, @aaron.ballman > wrote: > > > In D61835#1505280 <https://reviews.llvm.org/D61835#1505280>, @steveire > > wrote: > > > > > In D61835#1505228 <https://reviews.llvm.org/D61835#1505228>, > > > @aaron.ballman wrote: > > > > > > > In D61835#1505202 <https://reviews.llvm.org/D61835#1505202>, @steveire > > > > wrote: > > > > > > > > > 3. Anyone who wants traversal in the same way that dumping is done > > > > > and who needs to call API on the instance which is provided by > > > > > ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. > > > > > For example my UI. See my EuroLLVM talk for more: > > > > > https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/ > > > > > > > > > > > > Do they? Why is the `ASTNodeTraverser` insufficient? > > > > > > > > > It doesn't have the same behavior as `ASTDumper`, because `ASTDumper` > > > "overrides" some `Visit` metthods. > > > > > > I'm aware that they're different, but I may not have been sufficiently > > clear. I'm saying that the only public APIs I think a user should be > > calling are inherited from `ASTNodeTraverser` and not `ASTDumper`, so it is > > not required to expose `ASTDumper` directly, only a way to get an > > `ASTNodeTraverser` reference/pointer to an `ASTDumper` instance so that you > > get the correct virtual dispatch. e.g., `ASTNodeTraverser > > *getSomethingThatActsLikeAnASTDumper() { return new ASTDumper; }` > > > Perhaps. One way to implement the 'less noise' version of AST output (ie > removing pointer addresses etc http://ce.steveire.com/z/HaCLeO ) is to make > it an API on the `TextNodeDumper`. Then `ASTDumper` would need a forwarding > API for it. > > Anyway, your argument also applies to `JSONNodeDumper`, but you put that in a > header file. Yeah, and I was unhappy about doing so, but the alternative was to put it into ASTDumper.cpp as a local class and that felt even worse. > That was sane. We should move `ASTDumper` to a header similarly. (Perhaps a > rename of the class would help though?) Yeah, I'd be fine with renaming it to be a bit less general than `ASTDumper`. This LGTM. It's not a particularly clean interface, but I can see the utility in exposing it for D62056 <https://reviews.llvm.org/D62056>. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61835/new/ https://reviews.llvm.org/D61835 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
