[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

To me, the greatest advantage in using Expected<T> (ErrorOr<T> is the old way of doing things; I wouldn't use that in new code) is greater clarity in what are the possible results of the function. If the function returns the object and the error in two pieces, then this leaves place for uncertainties like "but what is the state of the T object if the function returned an error", or "if the T is invalid (null), can I assume the error object will indicate failure too", etc. Using Expected<T> removes all of these, because if the function returns an error, there is no T object to speak of and vice-versa. (Ok, if T = std::shared_ptr<U>, then this still leaves the possibility of the function returning "success", but with an empty pointer; however, that's still a much smaller problem space).

Whether the caller is able to do something with that error is not that important here. If he doesn't want it, he can always explicitly ignore it (or maybe log it?). So, I would say that changing this function to use Expected<T> is definitely a good thing, but it doesn't have to happen in this patch.

BTW, if you have a Status object, you can convert it to an Expected<T> (via llvm::Error) by calling ToError on it. Also, you don't have to define your own error type if you just want to return some string. StringError (llvm::createStringError) should suffice for that.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172