Re: [boost] [outcome] High level summary of review feedback accepted so far

Subject: Re: [boost] [outcome] High level summary of review feedback accepted so far
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2017-05-30 07:27:51

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.

>>  - 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.  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.

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.

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 */ }

(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.

> 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?

> 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.

> 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.