[PATCH] D61835: Extract ASTDumper to a header file

[Aaron Ballman via Phabricator via cfe-commits]

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