martong added a comment.
> 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.
Yes, we plan to submit a patch to LLDB too which would apply the use of
Expected<T>.
> If I understand you correctly, importNonNull and importNullable should work
> with Expected pretty well.
Yes, that's right.
> 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.
Yes I agree, that `Error` can help, and I also agree that this help is just
partial.
If we change `importNonNullInto` to has an `Error` return value
template <typename DeclTy>
LLVM_NODISCARD Error importNonNullInto(DeclTy *&ToD, Decl *FromD) {
if (auto Res = Importer.Import(FromD)) {
ToD = cast<DeclTy>(*Res);
return make_error<SomeError>();
}
return Error::success();
}
then on the call site, on the branch where whe handle the error we may make
mistakes like this:
CXXRecordDecl *ToTemplated = nullptr;
if (Err = importNonNullInto(ToTemplated, FromTemplated)) {
foo(ToTemplated->hasDefinition()); // Undefined Behaviour!
return Err;
}
If we do not use output parameters, only `Expected<T>`, then this could not
happen.
`Expected<T>.get()` aborts if there was an error.
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
