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