[PATCH] D61835: Extract ASTDumper to a header file

Thu May 16 11:34:36 PDT 2019

aaron.ballman added a comment.

In D61835#1505048 <https://reviews.llvm.org/D61835#1505048>, @steveire wrote:

> In D61835#1504663 <https://reviews.llvm.org/D61835#1504663>, @aaron.ballman wrote:
>
> > I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?
> >
> > It might help if I had a better idea of which APIs you thought were ones that would help users (because my only real concern with this change is that the public interface for this class is rather unpleasant).
>
>
> The reason the `ASTDumper` class still exists (for the purpose of dumping an AST to stream at least) is that it dumps the `{Function,Var,Class}TemplateDecl` 'correctly'.

Yup, this I recall.

> The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from `ASTNodeTraverser` on the instance. (That patch also extends the public API for users).

I don't see anything in the follow-up patch that actually uses the `ASTDumper` class though, so I'm still a bit confused.

> Perhaps some day the stream-dump output can be changed and the `ASTDumper` class will not be needed anymore. This is not that day :).

Yeah, and I'm not suggesting today needs to be that day, either. :-) It's just that this looks like a change with no positive impact because the change isn't being used anywhere (that I can tell), so the only thing I have to go on is "does this produce a decent public API?" and my feeling is "no, this class is an implementation detail and should remain hidden", especially since we think we can remove it someday.

Repository:
  rC Clang

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

https://reviews.llvm.org/D61835