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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor and Balazs,

> 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.

That's a bit disappointing because I was thinking that the status of error handling strategy discussion and implementation is reflected in the mailing list. But well, let's think what we can do 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.

Yes, I was thinking about it too. The reason why I chose `Optional` was that `ASTImporter` clients don't use the error kind. If you have any plans on changing this, `Expected` is a preferred approach.

> 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.

If I understand you correctly, `importNonNull` and `importNullable` should work with `Expected` pretty well. Changing `import*Into` return result to `Error` can partially help in avoiding usage of an unchecked result. These functions already guarantee that their parameters are changed only if the internal import was successful.

> 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.

Sure.


Repository:
  rC Clang

https://reviews.llvm.org/D50672