[boost] [outcome] Review

Subject: [boost] [outcome] Review
From: Bjorn Reese (breese_at_[hidden])
Date: 2017-06-02 20:28:13

I apologize in advance for the rather meek review below; I did not have
the time to be more thorough.

> - What is your evaluation of the design?

The review discussions provided many insightful arguments in favour
or against various aspects of the design, and I have not formed a
final opinion about most of these.

I do believe that fixing E for result<T> to error_code is the right
decision.

I would prefer a design without an empty state, because we already have
a vocabulary type for it.

The choice of C++14 seems motivated solely by performance
considerations. A slower C++11 version should be considered.

> - What is your evaluation of the implementation?

I did not have enough time to review it.

> - What is your evaluation of the documentation?

The introduction (Description) only provides a vague description of the
purpose of the library. For example, it does not mention that its types
are used to carry either a value or an error. Instead, it assumes that
the reader can derive this from prior familiarity with expected<T> (or
Rust.)

The first motivating example is too large (as indeed are the examples
in the tutorial.) Instead I suggest a "Tony table" that shows the
essense of the library (either value or error.) For example:

Before:

   FILE *file = fopen("myfile", "r");
   if (!file)
     throw std::system_error(errno, std::system_category(),
                             "Error delivered out-of-band by errno");
   fclose(file);

After:

   result<FILE *> file = fopen_wrapper("myfile", "r");
   if (!file)
     throw std::system_error(file.error(),
                             "Error delivered in-band by file.error()");
   fclose(file.value());

This may not look overly convincing, but it does demonstrate the either
value or error aspect in a few lines.

The library is weakly motivated -- the motivation boils down to a less
expensive alternative to throwing exceptions. A stronger motivation
would have been to mention use cases were errors cannot be propagated by
throwing exceptions. Here are two examples:

One motivating use case where exceptions cannot be throw are callbacks,
such as Boost.Asio asynchronous handlers. Here the return value is not
returned by the asynchronous operation (the initiating function), but
will be passed to the provided callback once the operation completes.
In other words, the following synchronous operation

   return_type operation(arg_type, error_code&);

is turned into this asynchronous operation:

   void async_operation(arg_type, void (*)(return_type, error_code));

Exceptions cannot be thrown into the functions (here the callback),
so errors have to be passed as arguments (whether error_code or
exception_ptr.) The above works if the return_type has a reasonable
"unengaged" value that can be passed as the first argument in case of
an error. While this is true for Boost.Asio, where return_type is a
std::size_t, it is not necessarily true in general. A much better
solution is to pass an outcome<T>, result<T>, or expected<T>:

   void async_operation(arg_type, void(*)(result<result_type>));

Another motivating use case is passing results between threads. For
instance, in the Actor model of concurrency information is passed
between actors/threads via a message queue. Some of these messages
are replies to requests that may or may not have failed. A queue
containing one of the Outcome types is well-suited for this kind of
interaction.

Apart from the above-mentioned shortcomings, I found the remainder of
the documentation quite readable.

I did not find the inheritance diagrams in the reference documentation
useful though.

> - What is your evaluation of the potential usefulness of the library?

The idea of an either-value-or-error return type is very useful.

> - Do you think the library should be accepted as a Boost library?

No, not in its current form.

I have two reasons for this rejection.

First, some of the design discussions (especially on narrow/wide
contracts) are not settled yet, so it would be premature to vote for
the design.

Second, whilst the lastest offer from the author about a fully-fledged
Boost.Outcome repository without a dependency on the non-reviewed
library called "boost-lite" is a step in the right direction, it is
unclear to me how that repository will end up. Having been surprised
by the current submission and the author's opinions, I will defer
any vote of acceptance until the a repository is submitted for review.