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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
