[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via cfe-commits]

martong marked 8 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:5039
+          if (!ToOrErr)
+            // FIXME: return the error?
+            consumeError(ToOrErr.takeError());
----------------
aprantl wrote:
> We don't typically commit FIXME's into LLVM code. Why not just deal with the error properly from the start?
Ok, changed that to return with the error.


================
Comment at: lldb/source/Symbol/ClangASTImporter.cpp:68
+      return *ret_or_error;
+    } else {
+      Log *log =
----------------
sgraenitz wrote:
> aprantl wrote:
> > The `else` is redundant.
> Here it's necessary for the scope of `ret_or_error`. That's a bit unfortunate. Maybe invert the condition?
Ok, I have inverted the condition and this way the else  became redundant, so I removed it.


================
Comment at: lldb/source/Symbol/ClangASTImporter.cpp:139
+
+      llvm::consumeError(result.takeError());
+
----------------
aprantl wrote:
> Can you convert this to an early return instead?
Okay, I have added
```
  if (!delegate_sp)
    return nullptr;
```
But we can't get rid of `consumeError` because otherwise the error object remains unchecked and will cause run-time assert.
`LLDB_LOG_ERROR` calls `consumeError` too plus logs the error msg, so I changed to use that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438