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

[Gabor Marton via Phabricator via cfe-commits]

martong added a subscriber: a_sidorin.
martong added a comment.

> I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.
>  Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

Hi @shafik ,

I completely understand your concern. We modify ASTImporter in order to make cross translation unit (CTU) analysis working on C++ projects. During these modifications we carefully try to keep LLDB functionality intact.

This patch is one of the last patches of a refactor in ASTImporter about using `Error` and `Expected<T>` as return types. We need this in CTU analysis because

- we want to distinguish between different kind of errors
- internally in ASTImporter we'd like to enforce the checking whether there has been any error during the import of any subexpressions

After this patch our next patch would rename these `Import_New` functions to `Import` and the old `Import` function implementations returning with a raw pointer would be deleted. This indeed would effect LLDB, thus we are going to create an LLDB patch too. We already have that LLDB change in our fork (https://github.com/Ericsson/lldb/commit/7085d20) and soon we will put that in Phabricator too.

Now, my concern is that waiting for the approve from @rsmith could take several months since he is usually very overwhelmed. Unfortunately, we have several other changes which depend on this change, so this is a blocker for us. Also, I think that @a_sidorin has the greatest experience in ASTImporter code. Would it be okay for you to accept this patch once Alexei approved it?


Repository:
  rC Clang

https://reviews.llvm.org/D53751