[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

Wed Nov 28 11:26:33 PST 2018

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