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

[Gabor Marton via Phabricator via cfe-commits]

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