Page MenuHomePhabricator

Add Status -- llvm::Error glue
ClosedPublic

Authored by labath on May 16 2017, 8:03 AM.

Details

Summary

I tried convert a function to llvm::Error/Expected but I quickly ran into the
issue of interfacing with code that still expects the Status objects.

This is my proposal for conversion functions between the two. I use
constructors and conversion operators on the Status class for the job,
but I make them explicit to make sure they're not converted
accidentally, as they have different semantics. I made sure conversion
preserves the errno error codes (by converting them to std::error_code,
which llvm::Error understands) as we have bits of code which switch
based on the error value. As for the other errors, only the error
message is preserved during the conversion.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 16 2017, 8:03 AM
zturner added inline comments.
include/lldb/Utility/Status.h
108 ↗(On Diff #99148)

I think we should remove the conversion operator. Instead, why don't we make a class called LLDBError and then just have people say:

Status S = getSomeStatus();
return make_error<LLDBError>(S);
source/Utility/Status.cpp
81–88 ↗(On Diff #99148)

Delete in favor of an LLDBError class as mentioned before.

zturner added inline comments.May 16 2017, 4:35 PM
source/Utility/Status.cpp
81–88 ↗(On Diff #99148)

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.

jingham accepted this revision.May 16 2017, 5:13 PM

This seems fine to me.

Zachary didn't give reasons why he didn't like the conversion form so I can't really assess that point. The use in the ErrorConversion test case seems pretty natural to me, but it's possible I'm missing whatever was bugging him about this. So I'll okay this for my part, but if you guys want to continue discussing, have at it!

This revision is now accepted and ready to land.May 16 2017, 5:13 PM
zturner edited edge metadata.May 16 2017, 5:34 PM

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.

If you're signing up to get an object that has strict usage semantics like llvm::Error, you had better be explicit about it.

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?

source/Utility/Status.cpp
81–88 ↗(On Diff #99148)

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.

labath updated this revision to Diff 99283.May 17 2017, 6:19 AM

use a separate function instead of a conversion operator

zturner added inline comments.May 17 2017, 9:07 AM
source/Utility/Status.cpp
81–88 ↗(On Diff #99148)

Yes, eventually we will definitely want to have different error implementations. CommandArgumentError, RemoteCommunicationError, DebugInfoError, etc all come to mind as potential candidates.

zturner accepted this revision.May 17 2017, 9:08 AM
This revision was automatically updated to reflect the committed changes.