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