[PATCH] D60910: [WIP] Dumping the AST to JSON

Mon May 13 13:33:36 PDT 2019

aaron.ballman added a comment.

In D60910#1495673 <https://reviews.llvm.org/D60910#1495673>, @rsmith wrote:

> If you're happy with these two conditions, then I have no concerns with this moving forward:
>
> - There is no implied stability for the content or format of the dump between major releases, other than that it be valid JSON; this should be stated explicitly in the documentation. (Compatibility between patch releases seems like something we can work out with the release manager, but I'm inclined to say we should make a best-effort attempt to preserve it.) If people want to build tools on this rather than on one of our stable APIs, they should expect to be broken in some way on every major release.
> - There is no requirement for people maintaining the AST (changing or adding AST nodes) to update the dump output for modified AST nodes to show any new information -- unlike the existing -ast-dump, this is not just for debugging, but we should be able to treat it as if it were. Perhaps a better way to put it: there is no requirement that the information in this dump is complete, but the information that is dumped should be correct.
>
>   If you want stronger guarantees than that, then we should have a broader discussion to establish some community buy-in.

Both of these conditions make a lot of sense and we're happy with them. I'll update the documentation accordingly as I land the initial patch. Thanks!

================
Comment at: include/clang/AST/DeclBase.h:1139
+  void dump(raw_ostream &Out, bool Deserialize = false,
+            ASTDumpOutputFormat OutputFormat = ADOF_Default) const;

----------------
steveire wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > I think we've talked about this before, but I don't think growing interfaces like this is the best way forward. An enum is a less-good replacement for an object (ie making the user of the API responsible for creating the dumper they want to use).
> > > 
> > > I think that could be made more convenient in the future. What do you think?
> > I think that may be a good future improvement, yes.
> > 
> > One thing to take on balance when considering this for the future is that `dump()` can be called from within a debugger and that's a bit harder to accomplish with an object interface. I'm not certain that will be a big issue for my use case, but it may matter to some folks.
> I don't think "it may matter to some folks." is a good guideline to use when designing an API. I have a patch which moves ASTDumper to its own header which I have just uploaded here: https://reviews.llvm.org/D61835
> 
> If additional args to `dump()` are mainly for debugging, then adding new args which are not for debugging doesn't seem right. 
The status quo is that we want `dump()` to be a function; changing that to use a different interface is certainly worth discussing. However, I'm opposed to trying to do that refactoring at the same time as I'm trying to land this functionality. I would recommend we land this initial patch first, then if we decide to refactor the `dump()` interface, do that in subsequent reviews.

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

https://reviews.llvm.org/D60910