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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
