JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.
LGTM
================
Comment at: lldb/source/Target/Target.cpp:2408-2446
llvm::Expected<lldb_private::Address> Target::GetEntryPointAddress() {
Module *exe_module = GetExecutableModulePointer();
llvm::Error error = llvm::Error::success();
assert(!error); // Check the success value when assertions are enabled.
if (!exe_module || !exe_module->GetObjectFile()) {
error = llvm::make_error<llvm::StringError>("No primary executable found",
----------------
dim wrote:
> JDevlieghere wrote:
> > I'm not particularly happy with the error handling in this function. The
> > patch seems correct, but as I've demonstrated it's easy to get this wrong.
> > I think we can rewrite the method as suggested to eliminate the issue. What
> > do you think?
> Yeah, I think that is much less error-prone. The `consumeError` method has a
> relevent comment:
>
> ```
> /// Uses of this method are potentially indicative of problems: perhaps the
> /// error should be propagated further, or the error-producer should just
> /// return an Optional in the first place.
> ```
>
> In this case it was just problematic to instantiate an `Error` right at the
> start, instead of when an actual error condition occurred.
>
> So I agree that your proposed approach is better. There is still one
> `consumeError()` in there, though?
> There is still one consumeError() in there, though?
I don't think that's true anymore?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86355/new/
https://reviews.llvm.org/D86355
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
