Re: [boost] [review] Review of Outcome (starts Fri-19-May)

Subject: Re: [boost] [review] Review of Outcome (starts Fri-19-May)
From: Robert Ramey (ramey_at_[hidden])
Date: 2017-05-27 00:22:35

On 5/19/17 6:43 AM, charleyb123 . via Boost wrote:
> Hi Everyone,
> 
> The formal review of Niall Douglas' Outcome library starts today
> (Fri-19-May) and continues until Sun-28-May.

I was sort of astounded at the volume of comment this submission 
received. I can't go through it all.  I can barely skim it.  Most of 
this went into implementation details which I wasn't prepared to invest 
the time to understand.  But it piqued my interest, so I resolved to 
write a review from the stand point of a naive user such as myself who 
might want to improve his approach to error handling.

I confess embarked on this task with a certain dread.  I had previously 
gone through Naill's documenation of AFIO and found it completely 
unintelligible.  Right away, I'll say that this is much, much better.

> Outcome is a header-only C++14 library providing expressive and type-safe
> ultra-lightweight error handling, suitable for low-latency code bases.

LOL - This is from the doc - and right away is off putting.  It includes 
a lot which may not be true.  I would have preferred something like:

"Output is a form of variant.  It can hold either some value type or 
some type indicating an error result.  Usage of Output will ease the 
coding and usage of functions which might return errors."

> Key features:
> 
> *- Makes using std::error_code from C++11's <system_error> more convenient
> and safe

The documentation provides useful explanatory information which isn't 
really available anywhere else in a convenient manner.  But lots of 
system_error aren't addressed - like a key part - error_condition.

It's not reallypart of outcome even though outcome supports it.  It's a 
separate subject. I think almost all reference to it should be dropped 
from outcome.  Useful information regarding error_code should be 
completed and added to boost::system_error via a PR to the boost.system 
library documentation.

> *- Provides high-quality implementation of proposed std::expected<T,E> (on
> C++20 standards track)

Personally, I find it distracting when authors describe their own work 
as high quality, or other laudatory phraseology. Spend the effort 
demonstrating the problems that it solves for me other rather than how 
great you believe it is.

> *- Good focus on low-latency (with tests and benchmarks)

The benchmarks seem to show that outcome is similar in performance to 
returning an integer or error_code.  This is what I would expect.

The benchmarks also purport to show that exception handling is much more 
expensive than returning an integer or error_code.  Well we know that if 
the exception is thrown.  But what if it isn't as is usual?  What about 
the cost of checking the error at each level?  This isn't addressed 
which is not surprise as it would be difficult to make a convincing 
benchmark which shows these numbers.  Since the timings for exceptions 
provide no useful information and the other ones are all the same, I 
would just drop the whole section and replace it with a sentence 
"outcome provides similar efficiency as returning a returning any other 
error indicator."

> *- Error-handling algorithmic composition with-or-without C++ exceptions
> enabled

I would drop just about all the references to exceptions.  It's an 
orthogonal issue.  If you've decided you need exceptions or that they 
are convenient in your context, you're not going to change to outcome. 
If you've already decided on returning error indicators (for whatever 
reason), you're not going to switch to exceptions.  You might want to 
switch to outcome.  So comparison with exceptions isn't really relevant 
at all.

Pretty much the same for error codes.  You might or might not use them - 
but this is a totally orthogonal question as to whether you use outcome 
or not as it is (I think) designed to handle any type as an error return 
types.

Eliminating all the irrelevant material would make the package much, 
much easier to evaluate and use.

FWIW - I touched upon some of these points in

https://www.youtube.com/watch?v=ACeNgqBKL7E

> *- No dependencies (not even on Boost)

Yowwww. This terrible.  I looked a little through boost-lite.
config.hpp - repeats a lot of stuff from boost/config.  Doesn't seem to 
provide for compilers other than gcc, clang and some flavors of msvc. 
Looks like some stuff might have some value - but with not docs/comments 
it's pretty much useless if something goes wrong - like a compiler upgrade.

chrono.hpp - what is this doing in here?

precompiling <tuple> etc.....   Looks like you're trying to solve some 
other problem here -

goes on an on.

The questions of pre-compiled headers/modules/C++11 subset of boost have 
nothing to do with outcome and shouldn't be included here.  Focus on one 
and only one thing: The boost.outcome library.  It's quite a small 
library and trying to make it carry the freight for all these other 
ideas will deprive it of the value it might have.

> This review is timely, as C++17 brings us std::optional<T>.  The upcoming
> std::expected<T,E> (an implementation provided in Outcome) is a
> generalization of std::optional<T> that provides a <success|fail> value,
> where the unhappy result is a 'std::error_code' or an instance of
> "your-chosen-error-type".
> 
> The library further provides 'outcome<T,error-code,exception-ptr>' for
> handling <success|error|exception> to safely wrap throwing APIs.

I didn't understand this either before or after reading the 
documentation.  I should confess that I don't understand the value of 
exception_ptr, nested_exceptions, etc. either. Another good reason for 
excluding discussion of exceptions and probably some code from the 
library documentation and code.

> Documentation:
>    https://ned14.github.io/boost.outcome/index.html

The documentation is built with Doxygen - which, though convenient, 
works against producing helpful readable documentation.

> Note:  Tarball might be easiest; but if you want to clone from GitHub
> directly, after the clone you should run the following command to get the
> source zip exactly:  git submodule update --init --recursive

Another bad idea.  Git is complex enough without adding submodules at 
the library level.  I would have to spend more time than I want to 
uderstand what good this is doing for us.  It's likely this would go 
away of the scope of the library were limited to what it should be: outcome.

> Please post your comments and review to the boost mailing list
> (preferably), or privately to the Review Manager (to me ;-). Here are some
> questions you might want to answer in your review:
> 
> - What is your evaluation of the design?
LOL - it's hard to discern the design from the docs and code.  But 
there's not many ways to implemented.  The questions I would have would 
be about never empty states and stuff like that but I don't see any of 
that in the docs.

Another very weird thing.  In spite of the size of the documents, there 
is not reference page for outcome<T, E> which tells me what ... OK 
looked harder and I did find it buried under Classes/Class 
List/outcome/v1_xxx/outcome.  In here I find a I have some questions:

typedef policy::monad_policy_base< policy::basic_monad_storage< 
policy::monad_policy< R > >, R, error_code_extended, std::exception_ptr 
 > ::implementation_type	implementation_type

What the heck is this - and why would I need to know it?  It's marked 
public which suggests that as a user I might need to access it - but the 
documentation does tell me what it does or why I would like to mess with it.

OK - now I turn to the class definition of outcome.  I see a boatload of 
constructors.  Hmmm I had thought that outcome was a template of the form:

boost::outcome<T, E> indicating that it returns a type T or E.

But no where do I see this.  Perhaps I'm wrong about outcome being a 
template.

I expect to see type requirements (aka concepts) for T and E.  Can any 
types be substituted?  It's a mystery.  Hmmm maybe initial example 
and/or looking at the source code will be useful.  First of all, the 
example has no #include <output.hpp> or some such.  I go looking for 
that and ....  I can't find it.  It's somewhere hidden through a maze of 
... preprocessing?

> - What is your evaluation of the implementation?

Needs to be slimmed down and all the extraneous stuff removed.  This 
isn't a vehicle for upending boost.  This is trying to get as modest 
simple header only library into boost in a way which makes it useful to 
users nothing more.
> 
> - What is your evaluation of the documentation?
A lot better then I expected!

> - What is your evaluation of the potential usefulness of the library?
I think it would be useful.  It's basically a specialization of variant. 
  As such it doesn't bring much we don't already have.  BUT, a good 
explanation and simple usage would promote a good and consistent way of 
addressing returned error codes ans so might be useful
> 
> - Did you try to use the library? With what compiler? Did you have any
> problems?

I only read the documentation and perused the source code.

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

A couple of hours reading the docs and source code.

> - Are you knowledgeable about the problem domain?

somewhat.  I understand error_code and have implement a version of this 
facility to address a need in my own code.
> 
> And most importantly:
> 
> - Do you think the library should be accepted as a Boost library?

No

There is a temptation to say Yes - with conditions. I would hope that 
this temptation is resisted.  It should just be rejected.  There is no 
way that this library can be fixed without separating out huge parts of it:

preprocessing all the headers - it's very hard to follow this, it even 
looks like it includes parts of the standard library as well as boost. 
What if every library did this?

error_code looks intertwined at least in the docs.

The class versioning scheme is interesting.  I could live with it but 
I'm not convinced that boost should adopt it.  And I'm less convinced 
that just one library - especially one that looks so simple as outcome.

If one thinks I'm being two harsh, I propose the following experiment:
Take a pair of left over interns from C++Now and direct them to the 
outcome git hub package.  Give them a simple task to use the outcome 
library.  See if they can complete it in one hour or less.

On the other hand, I believe it wouldn't be too tough to take the pieces 
of this library already made and compose them into a more tightly 
focused package that would fit into boost in a natural way that users 
would useful and easy to use.  But the look and feel of the package 
would be entirely different than the current one so It would really have 
to be reviewed again.  I also believe that if the package were 
re-formulated as I suggest, the review process would be much, much less 
onerous for everyone involved.

FWIW - my views on what a library has to have are summarized here:
https://www.youtube.com/watch?v=ACeNgqBKL7E