[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue

[Zachary Turner via lldb-commits]

Having the construction accept an Error is probably fine, because you have
to std::move it anyway, which means you know you're giving up ownership
On Wed, May 17, 2017 at 3:08 AM Pavel Labath via Phabricator <
reviews at reviews.llvm.org> wrote:

> labath added a comment.
>
> In https://reviews.llvm.org/D33241#756921, @zturner wrote:
>
> > Mostly just that implicit conversion operators are a very subtle source
> of bugs, and you can't even find where they're being used because it's
> impossible to grep for it.
>
>
> I agree, and that's why I made the conversions explicit. For example
> `Error foo() { return Status(); }` (or vice-versa) will fail to compile
> without an explicit cast. So you should always see that a conversion is
> happening as the other type in the general vicinity of the conversion.
> However you will not be able to easily grep for the occurences of the
> conversion, as you would be in the `toError` case.
>
> I did also have my doubts about whether this would be explicit enough
> though, so I would not be opposed to going to the separate function you
> propose. Do you want to beef up the explicitness on both directions of the
> conversion (e.g. `Status s = Status::ConsumeError(std::move(error))`) or
> just the one?
>
>
>
> ================
> Comment at: source/Utility/Status.cpp:81-88
> +Status::operator llvm::Error() {
> +  if (Success())
> +    return llvm::Error::success();
> +  if (m_type == ErrorType::eErrorTypePOSIX)
> +    return llvm::errorCodeToError(std::error_code(m_code,
> std::generic_category()));
> +  return llvm::make_error<llvm::StringError>(AsCString(),
> +
>  llvm::inconvertibleErrorCode());
> ----------------
> zturner wrote:
> > zturner wrote:
> > > Delete in favor of an `LLDBError` class as mentioned before.
> > Actually this doesn't really work, because you don't want to call
> `make_error<LLDBError>(S)` if `S` is a success condition.
> >
> > So perhaps instead, I would at least call it something more explicit.
> `llvm::Error toError() const` perhaps.
> Besides the success case issue, I believe having a member function (be it
> a conversion operator or not) allows us to have better interoperability
> between the two. I think of `Status` as being at the same level as
> llvm::Error, as it also tries to multiplex multiple error kinds into a
> single type, whereas having LLDBError introduces a subordinate
> relationship. Having a member function allows us to represent errno errors
> the same way as llvm does it.
>
> Instead of an LLDBError I'd propose we create more specialized error
> classes (UnwindError? NetworkError?) once we actually identify use cases
> for them. Although we can have LLDBError as a common superclass of these
> errors if you think it will be useful.
>
>
> https://reviews.llvm.org/D33241
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170517/990b2ade/attachment-0001.html>