[PATCH] D61835: Extract ASTDumper to a header file

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

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. That was sane. We should move `ASTDumper` to a header similarly. (Perhaps a rename of the class would help though?)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61835/new/

https://reviews.llvm.org/D61835