[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hi Aleksei,

Thank you for this patch.
With Balazs, we are working on something similar, but with a different, fine grained error value mechanism. Unfortunately we were not aware of that you have been working on error handling, and we didn't say that we are working on error handling recently, I am terribly sorry about this.

>From the CSA perspective, we realized that there may be several different error cases which has to be handled differently in `CrossTranslationUnit.cpp`.
For example, there are unsupported constructs which we do not support to import (like a struct definition as a parameter of a function).
Another example is when there is a name conflict between the decl in the "From" context and the decl in the "To" context, this usually means an ODR error.
We have to handle these errors in a different way after we imported one function during CTU analysis.
The fact that there may be more than one kind of errors yields for the use of the designated LLVM types: `Error` and `Expected<T>`. A simple `Optional` is probably not generic enough.

I find the `importNonNull` and generally the new family of `import` functions useful, but I am not sure how they could cooperate with `Expected<T>`. Especially, I have some concerns with output parameters.
If we have an Expected<T> as a result type, then there is no way to acquire the T if there was an error. However, when we have output parameters, then even if there was an error some output params could have been set ... and those can be reused even after the return. If error handling is not proper on the callee site then we may continue with stale values, which is not possible if we use Expected<T> as a return value.

Do you think we can hold back this patch for a few days until Balazs prepares the `Expected<T>` based version? Then I think we could compare the patches and we could merge the best from the two of them.


Repository:
  rC Clang

https://reviews.llvm.org/D50672