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

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

Le 21/05/2017 à 15:59, Niall Douglas via Boost a écrit :
>> A minor issue
>>
>>      * "As inferred in the example code cases earlier, you can check if
>>      an |expected<T, E>| has an expected value via a simple test for
>>      boolean true or more explicitly via |.has_value()| before retrieving
>>      the value using |.value()|."
>>
>> This is not correct. It is correct for the de-reference operator, but
>> .value() don't needs any check with .has_value().
> If you don't check before using .value(), it may throw
> bad_expected_access<E>.
So you have to fix the text.
>
>> Instead of some deviation, I would expect a report of all deviations and
>> a rationale explaining why it is better to do this way.
> I believe the list of deviations is complete, apart from the defects you
> found listed below which I will fix. Explaining why I deviate in detail
> isn't appropriate for the Outcome docs, I have raised why with you
> privately at length. Depending on what is decided at Toronto, I may
> write up remaining concerns into a WG21 paper, but in the end Outcome's
> Expected will track yours more closely than it has, so whatever you
> decide I'll do too.
My idea is to forward your concerns to Toronto, but for that I need 
arguments that I expected to find in this section.
>
>> # "Types |T| and |E| cannot be constructible into one another. This is a
>> fundamental design choice in basic_monad to significantly reduce compile
>> times so it won't be fixed."
>>
>>    * "Note though that |expected<void, void>| is permitted (which is
>>      legal in the LEWG proposal)."
>>
>> What are the compile time cost |associated the
>> std::experimental::expected<T,E>?
> Outcome doesn't do SFINAE on its main constructors. It relies on simple
> overloading which is much lower compile time cost.
Maybe it has lower compile time, but it is not correct, isn't it?
Would you suggest that std::expect shouldn't do SFINAE?
>
>> I don't see what |expected<void, void> could mean.
> It's legal in the LEWG Expected proposal. It's essentially a boolean.
Hugh!
>
>> # |unexpected_type<E>| is implemented as an |expected<void, E>| and it
>> lets the basic_monad machinery do the implicit conversion to some
>> |expected<T, E>| through the less representative to more representative
>> conversion rules. Note that our Expected passes the LEWG Expected test
>> suite just fine, so you probably won't notice this implementation detail
>> in your code.
>>
>> The test of my POC implementation couldn't be complete, so this is not a
>> sign of compatibility.
>>
>> Which operation allow to convert expected<void,E> to expected<T,E> and
>> what is the expected behavior if expected<void,E> has a value?
> basic_monad, separate to Expected, implements implicit conversion from
> any less representative form to any more representative form. So:
Could you point me to the documentation?
>
> result<void> => result<T>
> result<T> => outcome<T>
> expected<void, X> => expected<T, X>
>
> ... and so on.
>
> That's why implementing unexpected_type as an alias of expected<void, E>
> "just works".
>
> A current outstanding bug
> (https://github.com/ned14/boost.outcome/issues/16) is failure to compile:
>
> result<void> => result<T>
>
> ... when T has no default constructor because a valued void state turns
> into a default constructed T state. That *should* fail to compile of
> course, but the as_void() free function also fails to compile that, and
> it should work because otherwise the BOOST_OUTCOME_TRY() machinery fails
> to compile when the T of the enclosing function doesn't have a default
> constructor. It's my next issue to fix, it'll be done soon.
But what is the meaning if the result<T> if result<void> has a value?
>
>> # Our |make_expected()| and |make_unexpected()| functions deviate
>> significantly as we believe the LEWG ones to be defective in various
>> ways. This topic will be discussed in Toronto and reconciliation later
>> is highly likely.
>>
>> Please, could you tell how those are defective so that we can fix them?
>> Otherwise I don't see what we could discuss in Toronto.
> The most important problem by far is the inability to do
> make_expected<const Foo>(Foo()) => expected<const Foo>. I suppose const
> volatile also should be supported. I have already raised this with you,
> you said it will be discussed at Toronto.
Yes, it will.
This is not a good reason to don't provide what already is in or what we 
discussed together.
>
> Whatever you end up doing to make_expected() etc to enable const T and
> const E, I will replicate in Outcome's Expected. Until then I deviate to
> allow those. expected<const T, const E> is useful sometimes.
How you deviation allows it in a better way?
>
>> # 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?
> 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?
>
>> # Our Expected always defines the default, copy and move constructors even
>> if the the type configured is not capable of it. That means
>> |std::is_copy_constructible| etc returns true when they should return
>> false. The reason why is again to significantly improve compile times by
>> hugely reducing the number of templates which need to be instantiated
>> during routine basic_monad usage, and again this won't be fixed. Instead
>> use the static constexpr bools at:
>>
>>
>> Defining these operation when they can not be implemented means that you
>> cannot use SFINAE on this type. Maybe I'm wrong, but I believe we want
>> to check if a type is constructible, ... using the C++ standard type
>> traits.
> Correct. static constexpr bools provide the information instead. If
> reviewers like it, we may inject correct answers into namespace std for
> the type traits (this is permitted by the C++ standard).
Could you elaborate?
>
>> Next follow the first questions I have mainly from the reference
>> documentation of expected<T,E>.
>>
>> In general I find most of the operation not specified enough. I would
>> like to have something in line with the standard way of specifying an
>> interface.
> Already logged to https://github.com/ned14/boost.outcome/issues/26
>
>> * 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?
>
>> |* |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?
>
>> |"outcome<T>| can be empty, or hold an instance of a type |T|, or hold
>> an instance of |error_code_extended|, or hold an instance of
>> |std::exception_ptr|. This saves you having to wrap an error code into
>> an exception pointer instance, thus also avoiding a memory allocation."
>>
>> std::experimental::expected requires that its Error type to be no throw
>> constructible, copyable and movable.
> Those static asserts are gone from develop branch as we now fully
> implement the never empty guarantee for Expected.
Is there
>
>> 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?
>
>> 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 ...
>
>> * inplace construction
>>
>> What is the difference between:
>>
>> constexpr     expected (value_t _)
>> noexcept(std::is_nothrow_default_constructible< value_type >::value)
>>       Implicit constructor of a valued monad (default constructed)
>>
>> and
>>
>> constexpr     expected (in_place_t, Args &&... args)
>> noexcept(std::is_nothrow_constructible< value_type, Arg, Args... >::value)
>>
>> when there is no Args. IMO the second subsumes the first and so there is
>> no need fro the first overload and the value_t tag.
> The former has lower compile time load for valued default constructing
> an instance, and it's the constructor used internally by helper
> functions etc.
Maybe this should be private and these helper functions friend.
Id expected is default constructible, why do we need it? What is the 
difference between

constexpr     expected ()
and
constexpr     expected (value_t _)

?
> The latter is there for emplacement construction for
> those willing to pay the compile time cost.
>
>> * default constructor
>>
>> constexpr     expected () noexcept(is_nothrow_default_constructible)
>>       Default constructor.
>>
>> This doesn't say what the default constructor does.
> As per LEWG Expected, T is default constructed. The reference docs will
> be fixed with issue #26.
I insists #26 has nothing to do with constructors and I'm (we're) 
interested in knowing how this will be fixed.
>
>> * Shouldn't
>>
>> constexpr     expected (in_place_t, std::initializer_list< U > l)
>> noexcept(std::is_nothrow_constructible< value_type,
>> std::initializer_list< U >>::value)
>>       Implicit constructor from an initializer list.
>>
>> be
>>
>>    constexpr explicit expected(in_place_t, initializer_list<U> il,
>> Args&&... args);
> Ah yes I had forgotten about that.
>
> 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.
>
> Also, it's logged to https://github.com/ned14/boost.outcome/issues/27.
>
>> * What is void_rebound and where it is defined
> It was accidentally omitted from the reference docs. It is "using
> void_rebound = rebind<void>". Logged to
> https://github.com/ned14/boost.outcome/issues/28.
>
>> * construction from error type
>>
>> 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)

?
>
>> * 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.
>
>> * 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?
>
>> * What is expected<Policy> in
> Looks like a misparse by doxygen. Such a thing doesn't exist. Logged to
> https://github.com/ned14/boost.outcome/issues/30
>
>> * bool conversion should be explicit
> Another doxygen bug. Logged to
> https://github.com/ned14/boost.outcome/issues/31.
>
>> * is_ready has nothing to be with expected.
> Already removed from develop branch.
>
>> * Shouldn't value_or return by value? otherwise what would be the
>> difference between get_or and value_or?
> The get_*() family of functions is scheduled to be removed as per peer
> review feedback by https://github.com/ned14/boost.outcome/issues/24
Okay.
>
>> * emplace. Missing overload
>>
>>
>>      template <class U, class... Args>
>>          void emplace(initializer_list<U>, Args&&...);
> Great catch! Logged to https://github.com/ned14/boost.outcome/issues/32
>
>> * Missing preconditions on the observers, which make them wide
>> operations. Saying that they return an address makes them wide
>> operations however accessing to the contents of this address will be
>> undefined behavior.
>>
>> I prefer the standard way, defining a narrow function and stating
>> clearly the pre-conditions.
> Issue #26 will fix these.
>
>> * what error_or
>> <https://ned14.github.io/boost.outcome/structboost_1_1outcome_1_1v1__xxx_1_1policy_1_1expected__policy__base.html#ae0882c7d380b88b9011bcc60c59903bd>
>> returns if it contains an exception?
> The or parameter. Errored does not include excepted, but excepted
> includes errored because an error code is emitted as an exception_ptr to
> a system_error containing that error code.
Hugh! Too complex.
>
>> * Missing overload for get_unexpected. In addition it doesn't return an
>> unexpected_type.
>>
>> constexpr const E &     get_unexpected () const;
> I didn't realise there were rvalue, lvalue, const rvalue and const
> lvalue overloads. That must be a recent thing. Logged to
> https://github.com/ned14/boost.outcome/issues/33.
>
> 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.
>
> (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.
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.
This something that I remarked since the beginning of your library. The 
basic_monad class is over-engineered IMHO.
> 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.

Best,
Vicente