[boost] [outcome] Andrzej's review

Subject: [boost] [outcome] Andrzej's review
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2017-05-31 15:17:57

Hi Everyone,

This is my review of Boost.Outcome library. First, I want to thank Niall
for taking the effort to write the library, promoting it, sharing with the
community, and submitting for Boost review. I think that the library has
already proven useful regardless of this review's outcome (or should I say
'result').

I recommend to REJECT the library at this point, and encourage the author
to re-submit it after the list of what I consider issues -- described below
-- has been addressed.

Why reject and not conditionally accept? It is possible to say that I
accept the library if this "delta" is applied to the current submission's
state. Niall has indicated a couple of times that he is willing to apply
even big changes, if this is what reviewers wish. But the amount of change
that would be required, plus the uncertainty what the "delta" actually is,
plus a general confusion as to what library is for, as reported by others
(the effect of the current shape of documentation), in total make the
conditional acceptance too fragile.

What are we reviewing? It would be fair to assume that we are reviewing the
master branch, SHA 4851926c1c601c0e9391b2d0d07fb17f634b3ecb, but there is
also a "delta" described by Niall in high-level in one of the messages:
http://boost.2283326.n4.nabble.com/outcome-High-level-summar
y-of-review-feedback-accepted-so-far-td4694767.html. So maybe the master +
that delta. It describes that we will have like 10 types, like outcome_e,
checked_outcome_e, etc.. Now in another thread I hear there will only be 5
types: optional_outcome and outcome. This makes me believe that the
compromise solutions and promises are being made in a hurry, and this will
likely result in (1) suboptimal design and (2) misunderstandings as to what
we are actually agreeing to.

Am I knowledgeable about the problem domain? If the domain is "reporting
function failures in predictable-latency environments", then no, I have no
experience with this domain. I also have no experience with `expected`,
although having worked on formal specification of `std::optional` I am
familiar with a number of issues that `expected` also needs to solve. Also,
a number of controversies revolves around git sub-modules, dual licensing
and Boost distribution process. I do not feel competent in this area either.

How much effort did I put into the evaluation? I have read a number of
versions of the documentation. Had a long email exchange with Niall in
order to understand what the library does. I had a glimpse at the source
code to figure out the pieces that were missing from documentation. I have
tried running my own test examples on GCC 6.3.0 with MinGW w64 5.0.0
(doesn't work) and GCC 6.3.1 on Fedora (works fine).

Is the library useful? Yes. Apparently it solves an important problem in
predictable-latency environments. This is confirmed by the user base the
library already has managed to acquire. It provides an implementation of
the to-be standardized `exceptional`, which allows for experimentation. I
believe Boost would benefit from including this library (after the issues
have been resolved). I am confident Niall has knowledge about problem
domain.

Documentation.

It has two serious deficiencies. It lacks an introductory part that would
clearly and shortly describe what problem the library is solving an how,
how can I use it and when should I use it. It is also missing a reference
with documented semantics. One cannot tell what outocme's default
constructor does, or what function error() does when you have an
exception_ptr inside. I believe Niall when he says his users do not need
this documentation: they immediately grasp what and how it is doing. But of
the Boost library I expect a high-quality documentation.

I think that these deficiencies in the documentation prevent a successful
review of the library. I bet there are people who were put off by the docs
and will not proceed to reviewing the library further, even though they
could have given you many valuable insights, or point out defects. Some
will stop at criticizing the docs. Any potential flaws in the library will
not even be revealed. This may be the reason why things like ring buffer
were not opposed: the potential critics may have not gotten that far. This
is one of the reasons that I would recommend fixing the docs and staring
the review again without the distraction.

On the other hand, there is a lot of useful material in the documentation.
For sure it is a way better that the original versions. For instance, the
usage of ring buffer is documented clearly.

Design.

The idea to represent either a returned value or a reason for the lack
thereof as `variant<T, error_code, exception_ptr>` is sound. Fixing the `E`
to a single type is sound. using_error_code extended (along with ring
buffer) for carrying payload is sound. I also like the fact that you only
have three "monads": `outcome`, `result` and `expected`, and that they are
maximally different: expected and result make the opposite decision on
practically any aspect. This is a reasonable compromise between many
possible combinations of micro-decisions, and the expectation to have a
small number of types. `option` should go, I think. I can think of no use
case for it.

However, the design still changes throughout the review, and has not
stabilized yet. The last message is that we may have varieties for narrow
versus wide contract or we will have duplicated observers, and this has not
been settled on yet.

The consequence of using the ring buffer, where the error message can
suddenly disappear is unfortunate, but then again, I know of no way to make
it better. It might be the best choice of those available. I wish someone
from low-latency users stepped in and commented on whether this is the
right choice and whether the choice of these particular members (a short
string, two unsigned ints) is the optimum in practice.

Some aspects of design, to me seem unclear. E.g., what does it mean that
`outcome` has value but I am calling `o.error()`? Is it a programmer error,
which the function is trying to conceal, or does `o.error()` mean "convert
`outcome` to `error_code`, whatever the state? Similarly, when
`exception()` also recognizes error_codes, and `error()` does not recognize
exception_ptrs looks quite random: I cannot see a mental model behind it.
You could document detailed semantics of each function, but this would not
be enough. I need a clear mental model, like "every error_code or
exception_ptr is collectively called 'exception' in this library".

If you do not support `outcome<T&>, I need a clear indication of it and a
rationale.

Implementation.

These dual licensing, testing and git sub-module issues. I am not an expert
in how Boost is organized, but a number of people I consider qualified have
reported strong objections, and this has to be considered. A Boost library
must be compliant with "the way things work in Boost" even if it is felt
that things work wrong in Boost.

I did not look much, at the implementation, but it looks acceptable on the
surface. The assignment (most tricky part, I think) is implemented quite
clearly.

Sometimes static_assert's constrain too much. A non-default-constructible
types should be allowed. Types that throw upon copy/move construction
should be allowed provided I do not call the assignment on outcome/expected.

I am told `outcome<error_code>` does not work. I am not sure if this is
fortunate. It is no longer an `outcome<T>` that works for any Regular T.
What if I have a function that optains an error_code and might potentially
fail. How can I express this in the interface? (Or do I have to wrap in
`outcome<unique<error_code>>`?)

`outcome` has function swap overloaded in namespace std. I am not sure if
it is legal, but even if so, I would recommend implementing it in
`outcome`'s namespace, in order not to mess with `std`.

Again, thanks Niall for this library, and for a very educational discussion.

Regards,
&rzej;