Subject: Re: [boost] [outcome] High level summary of review feedback accepted so far
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-05-30 15:39:42
Le 30/05/2017 à 09:27, Gavin Lambert via Boost a écrit :
> On 30/05/2017 18:45, Vicente J. Botet Escriba wrote:
>> Le 30/05/2017 à 01:41, Gavin Lambert a écrit :
>>> FWIW, my preferred options would be (which I think were the originals):
>>
>> Thanks, you are the first (I believe) that supported the proposed
>> interface, but maybe I'm wrong.
>
> I think my suggestions aren't quite identical (in particular I think
> empty is handled differently in some cases). But they're close.
I don't want to have anything that sometimes is success and sometimes is
failure.
>
>>> - value() throws if holding error or exception (or empty)
>>>
>>> - error() returns E{} if holding value or E{errc::has_exception} if
>>> holding exception, or E{errc::no_value} if empty (names made up on
>>> the spot, doesn't matter)
>>
>> Would you like to extract the error code from the stored exception If
>> the exception had an error_code() function?
>
> If that's fast and noexcept, maybe. Otherwise it's probably not
> worthwhile. It's probably a coding error for a method that wants
> error_codes to call something that throws system_errors.
>
>> So the error codes must contain also the success codes. And now we
>> don't know if errc::no_value means success or failure.
>
> Niall's stated in other threads that in actual usage, sometimes empty
> is a success and sometimes it's a failure. So the decision of which
> it is has to be left to the caller, who presumably knows what it
> actually means.
>
> I expect that in a handling-empty code path, error() would never be
> called, or if called, would be stored in a variable that is later
> discarded. But the suggested behaviour makes it safe to use even if
> the code doesn't check for empty and does make use of the error code.
>
>> Wondering id make_error_code isn't more appropriated. I know you
>> don't like long names, but this is what the function is doing.
>
> In this context I'm describing behaviour absent of any judgement on
> what the things are actually called. Though FWIW both names are a
> little suspect since you'd expect make_error_code to actually return a
> std::error_code.
And it will.
error-code ec = r.make_error_code();
> make_unexpected from your proposal is more sensible in that regard; I
> suppose make_error_result or something like that would be more correct
> in this case.
I was talking of a factory from the result or the outcome. It was a
renaming of your error()
>
> Doesn't really matter too much as people can "using" bikeshed their
> own names in fairly easily.
>
>>> - exception() returns nullptr if holding value or error or empty
>>>
>> Why not create an exception when there is not, for the other cases?
>
> I can see an argument for returning a std::exception_ptr containing a
> std::system_error in the case where there is an error code.
>
> However I'm not convinced that this is the correct choice. The
> purpose of having an error code is to indicate non-exceptional
> failure, so synthesizing an exception from one in this case seems
> dubious.
If the purpose of error is to indicate an non exceptional error, why do
we want to add error codes that mean value, no_value and exception?
>
> Additionally, at least in MSVC any thrown exception (or exception
> captured by exception_ptr and rethrown, to a more limited extent)
> automatically carries a context from the original throw site (which
> can be turned into a stack trace). While this is still somewhat true
> of an exception_ptr constructed after the fact, it will capture the
> context at that point and not where the error was originally raised,
> which is perhaps misleading. Similarly for first-chance debugger
> intercepts.
>
> One possible advantage of doing that construction is that you could
> have code like this:
>
> if (r.has_error()) { std::rethrow_exception(r.exception()); }
>
> But without that, it's still possible with:
>
> if (r.has_error()) { (void) r.value(); /* will throw */ }
r.value(); // ;-)
>
> (Slight downside of this is that the compiler has to inline and walk
> into value() before realising that it always throws, so it might emit
> warnings unless you add some extra boilerplate.)
>
>> When you say that return nullptr you mean that it returns an
>> exception_ptr with a nullptr in.
>
> I meant literally return nullptr (exception_ptr is a NullablePointer
> so it's implicitly constructible from nullptr_t), but yes, it's
> basically the same thing, and the same as a default-constructed
> exception_ptr.
I know. I wanted to state that your are returning something that could
already be stored in a exception_ptr, and so the ambiguity starts. I
believe that outcome should store not_null_exception_ptr instead of
exception_ptr. not_null_exception_ptr is a thin class around
exception_ptr that doesn't provide default constructor.
>
>> Without this interface we can say the it is UB to have an
>> exception_ptr with a nullptr stored in an outcome. Now that we can
>> get one of those, it is much difficult to reason about.
>
> Reasoning about defined behaviour is surely easier than reasoning
> about undefined behaviour?
You don't need to reason about UB in your program, while you need to for
defined behavior. This means more code.
>
>> But I would like a single semantic for functions with the same name.
>
> What does this mean? I'm not sure how it applies here as we're
> talking about methods with different names.
You have error() functions for outcome, result and expected. Each one
doing one different thing.
>
>> So you are not one of those that needs the more efficient interface.
>> When you know the precondition is satisfied, why do you want to pay
>> for additional checks on the library and on the user code?
>> If you provide the minimal interface the user can built on top of it
>> whatever he needs. Not providing it will imply that all will pay for.
>
> Perhaps I am mistaken, but I generally prefer an overly-defensive
> coding style with explicit precondition checks on public interfaces.
> I don't trust developers (including myself) to write correct code and
> I don't trust them to run static analysers on their incorrect code.
We don't play in the same game. I'm not against safety, but I need
sometimes to master the efficiency. It is a pity if you cannot use these
classes because you are paying for what you don't use.
A library must provide the minimum, and the minimum is a narrow contract.
You could add the static analyzer run on the Continuous Integration (CI)
or as a necessary check before merging to the develop branch.
You can run your CI tests with sanitizers.
Vicente