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