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

Subject: Re: [boost] Boost.Outcome review - First questions
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2017-05-21 13:59:31

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

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

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

> I don't see what |expected<void, void> could mean.

It's legal in the LEWG Expected proposal. It's essentially a boolean.

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

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.

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

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.

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

The fact optional implemented .value_or() with a return by value is a
mistake. Expected need not repeat that mistake.

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

Vicente thanks very much for such detailed feedback. It was very valuable.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/