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

Gabor Marton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu May 9 07:25:43 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list