[Lldb-commits] [PATCH] D86355: Consume error for valid return from Target::GetEntryPointAddress()

Dimitry Andric via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Aug 21 11:49:47 PDT 2020


dim added inline comments.


================
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",
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86355



More information about the lldb-commits mailing list