Re: [boost] Boost.Outcome review - First questions

Subject: Re: [boost] Boost.Outcome review - First questions
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-05-22 05:54:54

Le 22/05/2017 à 01:15, Niall Douglas via Boost a écrit :
> Now to the reply to your post point by point ...
>
>>>> # Our |value_or()| member functions pass through the reference rather than
>>>> returning by value (we don't understand why the LEWG proposal does
>>>> except that |std::optional<T>| does, which itself is a bad design choice
>>>> and it should be changed).
>>>>
>>>> Well, what is the advantage to returning by reference?
>>> You don't cause .value_or() to surprisingly fail to compile when T lacks
>>> a move or copy constructor. The reference returning edition does not
>>> impose the requirement for T to have a move or copy constructor. This
>>> choice for .value_or() interferes less on end users.
>> Do you have cases where expected<T,E> has a T that is not move/copy
>> constructible?
>> How can you return it?
> You can't directly. But some might wrap it in a unique_ptr. Up to them,
> it is they who supply type T and they may not even control its
> implementation. If a user is forced to use such a T they cannot modify,
> I don't think you should punish them for that.
>
> In general I don't think you can rightly impose that T needs to have a
> move or copy constructor. You don't *have* to impose it, so don't impose
> it. Don't mess up the end user's life and make their programming harder
> for no good reason.
Niall, I will not be against your interface if I had a use case or if it 
can perform better. Providing a more complex interface because we can 
and in case someone can need it is not enough IMHO.
>>> The fact optional implemented .value_or() with a return by value is a
>>> mistake. Expected need not repeat that mistake.
>> Could you point me to the C++ standard std::optional defect? Have you
>> created one?
> optional is in the C++ 17 standard. Code will be written against its
> current design of .value_or(). It can no longer be changed :(
You found this 2 years ago, isn't it?
>
>>>> * it is not clear from the reference documentation if expected<void,E>
>>>> is valid?
>>> Do you mean there is no reference in the API docs to an expected<void,
>>> E> specialisation? I didn't add one as I figured it was self evident and
>>> unsurprising. Also, if we document expected<void, E>, we'd have to also
>>> do result<void>, outcome<void>, expected<void, void> etc and to be
>>> honest, they all behave exactly as you'd expect a void typed edition
>>> would: wherever there is a T, you get T=void. Nothing special.
>> Maybe or maybe not. What is the signature of value_or?
> Exactly the same: value_type &value_or(value_type &)
I believe these differences merit to be in the reference documentation.
>
> value_type is defined to an unusable but helpfully named type for
> compiler warnings and error when raw_value_type=void. This is very
> tersely explained in the Expected synopsis page. I will be improving the
> reference API docs to describe this better as part of issue #26.
>
>>>> |* |is expected<T,E> the sum type of T, E and empty? and exception_ptr?
>>> expected<T, E> may only have the state of T or E. The valueless by
>>> exception state has been eliminated recently in develop branch.
>> Is this the case also for outcome/result/option?
> Yes.
Great, so result<T> and option<T> wouldn't have no anymore a tri-logic, 
and outcome<T> would have a tri-logic as it can be T | E | 
exception_ptr, but never empty.
I'm not sure, but  I believe you maybe will need to double the storage. 
Have you implemented this already?
>
>>>> What are the exception warranties for |error_code_extended?
>>> It never throws, not ever.
>>>
>>> This is pretty clear in the reference docs. Everything is marked noexcept.
>> I don't see it in
>> https://ned14.github.io/boost.outcome/classboost_1_1outcome_1_1v1__xxx_1_1error__code__extended.html
>> Could you point me where noexcept is?
> Heh. You are completely right, noexcept is missing. That's a bug: logged
> to https://github.com/ned14/boost.outcome/issues/34.
>
>>>> What are the exception warranties for |outcome<T>||?
>>> The tutorial covers those. The reference docs will too when issue #26 is
>>> implemented. The noexcept modifiers on most of the member functions are
>>> pretty clear however.
>> This is the kind of information we want on the reference documentation.
>> Could you tell us what the issue #26 The default actions for each of
>> .value(), .error(), .exception() etc need to be spelled out in the
>> reference docs <https://github.com/ned14/boost.outcome/issues/26> will
>> add to the docs and how this issue is related?
>> What about the other functions, constructors, assignments ...
> The information in the tutorial will be copied into each reference API's
> docs using a style similar to standardese. I will cover constructors,
> assignments, destructors etc as well.
>
>>> You are correct, but I left it unfixed to remind me to ask here what
>>> would be a suitable unit test for testing an "initializer_list<U> il,
>>> Args&&... args" constructor where the Args&&... gets used correctly. I
>>> was unsure of the semantics.
>> Take a look at std::optional test if you are curious.
> I found only this in libstdc++'s unit tests for its optional:
>
> std::optional<std::vector<int>> o { std::in_place, { 18, 4 },
> std::allocator<int> {} };
>
> It'll do.
>
>>>> Why do you prefer an implicit conversion. This deviation must be in the
>>>> rationale of the differences.
>>> The deviations list already explains that types T and E cannot be
>>> convertible into one another because we use simple overloading to avoid
>>> using SFINAE to keep compile time load low.
>>>
>>> Code written for LEWG Expected will, unless types T and E are
>>> convertible into one another, work as expected.
>> You don't answer to my question. Why implicit?
>> What is wrong with the constructor
>>
>>      expected(unexpect_t, error)
>>
>> ?
> As I've mentioned to you by private email, a lot of end users want
> implicit conversion from T and E to save on boilerplate. Outcome doesn't
> allow T and E to be convertible into one another, so permitting implicit
> conversion from both T and E is safe.
Aside what we can have discussed privately, we are here reviewing the 
library. I believe this kind of information should be part of the rationale.
I would say the limitation you impose to T and E permit to define the 
constructor implicitly. This doesn't mean that we need to.
>
> Consider it a populist extension over LEWG Expected. I know you don't
> like it, and I to be honest think it best used sparingly, but it'll be
> very popular with many.
Popular is not  a criteria I will run after.
>
>>>> * raw types
>>>>
>>>> It is not clear what is the difference between xxx and raw_xxx
>>> Logged to https://github.com/ned14/boost.outcome/issues/29
>> Thanks for creating the issue, but I'm interested in knowing it now
>> during the review. Otherwise I couldn't accept even conditionally the
>> library.
> When configured with void, value_type, error_type and exception_type are
> all set to an unusable but usefully named types suitable for compiler
> warnings and errors. This makes writing metaprogramming much easier as
> you don't need to deal specially with void and its weird semantics, plus
> any attempt to use the type causes a very descriptive compiler error.
>
> raw_value_type, raw_error_type and raw_exception_type are the true,
> original type configured. You are correct that this is a deviation from
> LEWG Expected which would cause code written for LEWG Expected to fail
> to compile with Outcome's Expected. I think that safe.
If the raw_ are the reason d'être to avoid the void specialization, and 
this avers to be a useful technique, I believe it merits a full 
implementation section explaining how this improve the compiler 
performances, DRY, et all.
I'm not against something I don't understand, in principle, just want to 
understand why you did the way you did and if it is a good technique 
adopt it on my proposals.
>
>>>> * what is the sizeof expected <T,E>?
>>> It says already at the top of the page. It's max(24, sizeof(R)+8) on 64
>>> bit CPUs.
>> An now that you will ensure the never empty warranties?
> It's the same sizeof.
What are the magic number 24 and 8? I'm sure I can undertand it better 
if I read the implementation, but I don't I want to read the 
documentation and understand it.
>
> If all your types have nothrow move, or your types don't have move nor
> copy constructors, we directly destruct and construct during assignment
> without saving the old state as the fast path. Else we move the current
> state onto a stack based copy before copy or move constructing in the
> new state. If that throws, we move back the previous state. If that
> restoring move throws as well, we are left valueless by exception/empty.
>
> I didn't implement Anthony William's double buffer solution for perfect
> never empty warranties even when the types have no move nor copy
> constructors. It is quite complex if you also wish to get alignment
> right, constexpr right, and keep the optimiser maximally folding code.
>
> I will document the semantics described above in the tutorial and
> reference API docs. They are a good balance of never empty warranties
> with space consumption and runtime overhead. I believe these guarantees
> are similar to those in your Expected proposal? As in, you don't
> implement the double buffer solution either.
No, I don't need to. There is not need if E concerned function not 
throw, which I could expect as E is an error.
>
>>> The return by E rather than expected<void, E> is by design. Remember we
>>> permit implicit construction from E, so this is safe and works well.
>>> Again, code written for LEWG Expected should never notice this
>>> implementation detail.
>> Why? if the signatures doesn't match I will have sure a problem somewhere.
> Such problems are on me as the maintainer.
No the possible problem will appear on the user side as soon as the user 
make use of the signatures.
I agree that this can be a corner case.
>
>>> (the real cause for this deviation is because the policy classes cannot
>>> return types still being fabricated, so they cannot return an
>>> expected<void, E>. If this deviation ever should become a problem, I can
>>> relocate get_unexpected to the main class and enable_if it, or else use
>>> a trampoline type. But I believe the existing form replicates the same
>>> semantics, it should be fine)
>> This is what I don't like of your library. In order to have a common
>> implementation you sacrifice the concrete interfaces.
> The common implementation is the heart and soul of Outcome, and is
> precisely its "value add" over most other Either monad or Expected
> implementations. A common implementation allows seamless interoperation
> between implementations. It lets you write code using expected<T, E>
> with outcome<T>, result<T> and option<T> all using the exact same
> framework. End users can extend the family with any arbitrary custom
> error transport of their choosing for some bespoke use case.
Your solution is intrusive IMHO. How boost::experimental::expected<T,E> 
will interact with std::experimental::expected<T,E> or std::optional<T>, 
or something else that behaves like a monad error?
If we want different monad errors to interact between them we should 
build this on top of the monadic error interface. And IMO, we should do 
it explicitly as we do with exceptions when we transform some exceptions.
>
>> I prefer to have concrete classes that provide the interface is the most
>> adapted to these classes and then have an adaptation to some type of
>> classes requirement when I need to do generic code.
> In most of my libraries so do I. But in this situation, I went a
> different direction. I think it's the right one for this use case.
No problem. We just disagree here.
>
>> This something that I remarked since the beginning of your library. The
>> basic_monad class is over-engineered IMHO.
> Please see the other email for a reply to this sentiment.
It is late now. I will replay to it this evening.
>
>>> Vicente thanks very much for such detailed feedback. It was very valuable.
>>>
>> Niall, please, don't wait until the review is finished to tell us how
>> the issues will be fixed. You could do it on each issue and come back in
>> this ML.
> I try my best to do so. But a lot of time, how the issues will be fixed
> is very obvious: I copy and paste documentation from one location to
> another for example, or I repair an incorrect constructor so it is
> correct and so on. It's not rocket science to deduce how most issues
> will be fixed.
>
It could be obvious for you, but we need to review those possible 
changes before accepting the library.

Niall, I'm not against not for you library. We're just reviewing it now. 
For me the goal is to improve it, and if we can at the same time improve 
the std expected proposal this will be very valuable to the C++ community.

Best,
Vicente