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