[boost] Boost.Outcome review - First questions

Subject: [boost] Boost.Outcome review - First questions
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-05-21 10:33:57

Hi,

I've tried to read the tutorial, but it is too long for my taste. I will 
try it later on :(

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

Instead of some deviation, I would expect a report of all deviations and 
a rationale explaining why it is better to do this way.

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

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

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

# Our |expected<T, E>| defaults E to |std::error_code| rather than 
|std::error_condition| (the LEWG proposal fixes this in P0323R2).

I don't have a strong opinion on which default would be the best. As you 
suggested, I have added it to the next revision.

We don't implement the ordering and hashing operator overloads due to 
https://akrzemi1.wordpress.com/2014/12/02/a-gotcha-with-optional/. The 
fact the LEWG proposal does as currently proposed is a defect (it will 
be discussed by LEWG with P0323R2 in Toronto).

As for optional this could be considered a defect. The question is if we 
want expected to differen from the optional interface.

If we considered it a defect, I don't believe that we should remove 
comparison, but IMHO we should make the constructor from T explicit.

I don't expect the committee to fix this for expected and let the issue 
for optional. We will see.

# We don't implement |make_expected_from_call()| as we think it highly 
likely to be removed from the next version of the proposal due to it 
conferring little value.

This is possible and the function can be implemented very easily. I can 
live with and without it.

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

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

Rather than |void|, our |value_type| and |error_type| typedefs are set 
to a descriptive compiler error generating type if the Expected is 
configured with |void|. If you want to know if no type was configured, 
use the static constexpr bools at |expected<T, E>::has_value_type| and 
|expected<T, E>::has_error_type| or use |raw_value_type| and 
|raw_error_type|. The reason for this deviation is to make 
metaprogramming using Outcome's transports much easier and less error 
prone.

Sorry. I don't understand what is the issue. Could you elaborate?

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

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.

* it is not clear from the reference documentation if expected<void,E> 
is valid?

|* |is expected<T,E> the sum type of T, E and empty? and exception_ptr?

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

What are the exception warranties for |error_code_extended?
|

What are the exception warranties for |outcome<T>||?
|

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

* default constructor

constexpr     expected () noexcept(is_nothrow_default_constructible)
      Default constructor.

This doesn't say what the default constructor does.

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

* What is void_rebound and where it is defined

constexpr     expected (const void_rebound &v) 
noexcept(std::is_nothrow_copy_constructible< error_type >::value)
      Implicit constructor from an identically configured 
basic_monad<void> by copy.

* construction from error type

What is wrong with

     template <class... Args>
         constexpr explicit expected(unexpect_t, Args&&...);
     template <class U, class... Args>
         constexpr explicit expected(unexpect_t, initializer_list<U>, 
Args&&...);

Why do you prefer an implicit conversion. This deviation must be in the 
rationale of the differences.

constexpr     expected (const error_type &v) 
noexcept(std::is_nothrow_copy_constructible< error_type >::value)
      Implicit constructor from a error_type by copy.

constexpr     expected (error_type &&v) 
noexcept(std::is_nothrow_move_constructible< error_type >::value)

* raw types

It is not clear what is the difference between xxx and raw_xxx

typedef value_storage_type::value_type     value_type
      The type potentially held by the monad.

typedef implementation_policy::value_type     raw_value_type
      The raw type potentially held by the monad.

typedef value_storage_type::error_type     error_type
      The error code potentially held by the monad.

typedef implementation_policy::error_type     raw_error_type
      The raw error code potentially held by the monad.

typedef value_storage_type::exception_type     exception_type
      The exception ptr potentially held by the monad.

typedef implementation_policy::exception_type raw_exception_type
      The raw exception ptr potentially held by the monad.

* What is the "The exception ptr potentially held by the monad"?

* what is the sizeof expected <T,E>?

* is basic_monad something proposed or an implementation detail?

If the first, this should be renamed, if the second this class should 
not appear in the documentation and the word monad should be banned.

* What is expected<Policy> in

template<class Policy , typename = typename 
std::enable_if<std::is_same<typename implementation_policy::value_type, 
typename Policy::value_type>::value || std::is_void<typename 
Policy::value_type>::value || std::is_constructible<typename 
implementation_policy::value_type, typename 
Policy::value_type>::value>::type, typename = typename 
std::enable_if<std::is_same<typename implementation_policy::error_type, 
typename Policy::error_type>::value || std::is_constructible<typename 
implementation_policy::error_type, typename 
Policy::error_type>::value>::type, typename = typename 
std::enable_if<std::is_same<typename 
implementation_policy::exception_type, typename 
Policy::exception_type>::value || std::is_constructible<typename 
implementation_policy::exception_type, typename 
Policy::exception_type>::value>::type>
constexpr     expected (expected< Policy > &&o, 
explicit_conversion_from_different_policy=explicit_conversion_from_different_policy())
      Explicit move constructor from a basic_monad with a differing 
implementation policy. For this constructor to be available, value_type, 
error_type and exception_type must be identical or constructible.

* move constructor/copy constructor

The reference documentation doesn't says what this does.

* Missing move/copy assignment

* bool conversion should be explicit

* is_ready has nothing to be with expected.

* Shouldn't value_or return by value? otherwise what would be the 
difference between get_or and value_or?

* emplace. Missing overload

     template <class U, class... Args>
         void emplace(initializer_list<U>, Args&&...);

* swap: What is the meaning of swapping an expected with an option?

swap must follow the standard signature.

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

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

* what is the difference between get_error_or and error_or?

https://github.com/ned14/boost.outcome/issues/19

* Missing overload for get_unexpected. In addition it doesn't return an 
unexpected_type.

constexpr const E &     get_unexpected () const;

Best,

Vicente