[boost] [outcome] Andrzej's review

Subject: [boost] [outcome] Andrzej's review
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2018-01-26 18:17:13

Hi Everyone,
This is my review of Boost.Outcome v2. The library should be *accepted*
subject to conditions that I list down below.

What is your evaluation of the design?

Clean and logically divided.

What is your evaluation of the implementation?
>

Some bugs aside, it is clean and well structured. I was able to read it and
understand how things work. I will be filing bugs in GitHub.

What is your evaluation of the documentation?
>

Good. You can get the idea what the library is doing and how you can use
it. But I should mention that I wrote some pieces thereof, so my opinion is
likely biased. The docs need some modifications before they can get into
Boost. I mean macro names, and the technology required to build it.

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

I find the library very useful in certain domains. I myself have found two
applications in my job project, and they are not really related to
light-speed efficiency:

1. We have a task scheduling framework built atop ASIO, and signalling
failures through exceptions makes the code really difficult to read because
try-catch blocks need to work as control statements. Also, we cannot pass
them by exception_ptr as sometimes we need to classify the kind of failure
to determine if we want to deal with it in the framework, or if we want to
forward them to the user.

2. I have a validation routine that validates (and parses) the input data
written using some syntax. I wan the syntax-errors in input files to be
forwarded as failed `result`s (no exceptional about these), but other
errors like memory shortage during the file parsing to be signaled via
exceptions (and I will probably kill the module in response).

Why accepting this library when we have `expected` being standardized? I do
not really mind having two in Boost. First, it is ready, being proposed,
and applied in a number of non-trivial code-bases. Niall claims that his
trade-offs are better tailored to predictable-latency applications. I do
not have enough knowledge to asses it. They look convincing. The only way
to test it is to give this library the Boost blessing and have the user
test it. I am bought by the TRY operations, and the upgrade to `outcome`
looks really convincing.

Did you try to use the library? With what compiler? Did you have any
> problems?
>

Yes. A lot of compilers. For instance GCC 6.4.0, also different GCC and
Clang versions in WandBox. No problems encountered.

How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
>

I have read most of the final implementation. I have read many versions of
the documentation. I have spent a lot of time (prior to the review) asking
Niall lots of questions about the details. I have also built lots of small
examples to test the library and some corner cases.

Are you knowledgeable about the problem domain?
>

I do report failures (by different means) in my programs. I have given it
quite some thought. I am well familiar with implementation and formal
specification of `boost::optional` and `std::optional` which had to address
some similar implementation problems.

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

Yes, conditionally. Here are three conditions:

1. Documentation is adjusted to use boost-standard names of macros and
namespaces. Also, it is not clear if Boost release process is capable of
handling the technology that Niall has chosen. If not, the contents of
documentation should be migrated to another format, manageable by Boost
release process.

2. Because there has been some confusion around the meaning of storing
default-constructed `std::error_code` (is it an error or lack thereof?) and
`std::exception_ptr` (e.g., see this issue:
https://github.com/ned14/outcome/issues/115), I request that you state in
the docs, before the API Reference how library treats default-constructed
values of `EC` and `EP`. For instance, you can say "default-constructed
error_code is considered an error, and treated as such by the library", or
"the library sometimes may make use of the fact that a default-constructed
EC/EP is not an error, so make sure not to pass us such values": I am fine
with any answer, but I need you to spell it out, so that I know what to
expect.

3. Similarly, I request that you put in the docs what is the "value" of an
object of type `outcome<>` or `result<>` in the sense in which John Lakos
talks about "salient attributes".
I know that T, EC, and EP constitute the value. But in the middle of the
tutorial we learn that apart from these you also store "spare_storage" --
is it also included in copying and comparisons? API reference on
`operator==` only says "compares if two objects are equal", but it is not
that clear what it means. Also, what does it mean that result is equal to
optional. Define it somewhere and paste links to it.

Finally, I wan to thank Niall for his now many-year effort to deliver this
really cool library to the community.

Regards,
&rzej;