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

[Pavel Labath via Phabricator via lldb-commits]

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