aaron.ballman added a comment. In D53751#1311037 <https://reviews.llvm.org/D53751#1311037>, @martong wrote:
> > I didn't actually see this comment get addressed other than to say it won't > > be a problem in practice, which I'm not certain I agree with. Was there a > > reason why this got commit before finding out if the reviewer with the > > concern agrees with your rationale? FWIW, I share the concern that having > > parallel APIs for any length of time is a dangerous thing. Given that > > "almost ready to go" is not "ready to go" but there's not an imminent > > release, I don't understand the rush to commit this. > > @aaron.ballman Thanks for your time and review on this. > > I completely understand you concern and agree that having such parallel API > even for a short time is not good. Please let me explain why we chose to do > this still: > `ASTImporter::Import` functions are used externally by LLDB and CTU as > clients. However, the functions are used internally too, inside `ASTImporter` > and `ASTNodeImporter`. E.g. when we import an expression, then first we use > the `Import(QualType)` function to import its type. > Our goal is twofold: (1) enforce `ASTImporter` and `ASTNodeImporter` > implementation functions to always check the result of used import functions > and (2) make sure that clients can have detailed error information, so e.g. > in case of CTU we can handle unsupported constructs differently than ODR > errors. > As @balazske mentioned we could have changed the API in one lockstep but the > impact would have been too huge. > > In the context of this patch, you can think of the newly introduced > `Import_New` functions as the internal only functions. I was thinking about > that we could make them private and `ASTNodeImporter` as a friend, that way > we could hide them from clients and then the dual API would cease to exist. Thank you for the explanation -- I guess I would have preferred to go with the suggestion from @shafik to have done this one API at a time, rather than as an entire set of APIs. However, given that the code is already in, I don't think it would be worth the churn to revert and go that route. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53751/new/ https://reviews.llvm.org/D53751 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
