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

Subject: Re: [boost] [review] Review of Outcome (starts Fri-19-May)
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2017-05-27 13:29:03

On 27/05/2017 01:22, Robert Ramey via Boost wrote:
>> 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.

You should thank Reddit. My only contribution was not choosing to break
my arm instead of rewriting the docs for a fourth time.

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

Reddit feedback didn't like that description BTW, I did try it. And the
current description was not written by me in fact, it was donated
through dislike of my previous descriptions.

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

The entire of
https://ned14.github.io/boost.outcome/md_doc_md_03-tutorial_b.html is
about nothing other than error_condition and how to use it.

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

There have been repeated calls for hard numbers with repeatable
benchmarks others can run, usually with implication that I am lying to
people or deceiving them somehow by giving them an open source library
free of cost. I personally agree with you that such toy benchmarks are
not useful, but the people cry for red meat, if anything they want more
useless benchmarks and hard numbers.

Donations of more benchmarks are welcome!

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

You may have a missed a major use case for outcome: calling STL code in
a C++ project which is C++ exceptions disabled. Much of the games and
finance industry are interested in Outcome precisely because of the ease
Outcome enables passing through C++ exceptions thrown in a STL using
island through code with exceptions disabled.

Even if you're not doing that, another major use case is to keep
exception throws, and having to deal with control flow inverting
unexpectedly, isolated into manageable islands. The rest of your
intermediate code can then assume no exception throws, not ever. That
means no precautionary try...catch, no need to employ smart pointers, no
need to utilise RAII to ensure exception safety. It can be a big time
saver, especially during testing for correctness because it eliminates a
whole load of complexity.

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

You are thinking of expected<T, E> where E is selectable by the end
user. Outcome's outcome<T> and result<T> hard code the error type to
error_code_extended. So error codes are the only game in town for those
refinements.

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

With respect Robert, I don't think you understood all the use cases.
Most have called for significantly *more* documentation to cover in
detail even more use cases, not less.

(Incidentally I tried less documentation three times previously. For
some reason, people need to be hand held more when it comes to
describing really simple and obvious-to-me at least stuff)

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

It actually provides for all the C++ 14 compilers currently available.
If they are not MSVC, GCC nor clang, they pretend to be one of those, so
the same detection routines work.

You may also have missed the extensive use of C++ 17 feature test
macros. All C++ 14 compilers, even MSVC, support those now.

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

The C++ 17 feature test macros ought to be observed by all future
compiler upgrades.

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

I think you didn't realise those are the namespace bind files allowing
the C++ 11 STL to be switched with the Boost STL. They are auto
generated by a clang AST parser.

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

That would not be a widely held opinion. Most end users of Outcome don't
want any Boost connection at all. They do want excellent top tier cmake
support, with automatic usage of compiler technologies such as
precompiled headers, C++ Modules, clang-tidy passes, unit test
dashboarding, sanitisers, ABI guarantees, and lots of other goodies.
boost-lite provides lots of common cmake machinery used by all my
libraries to save me reinventing the wheel for each.

Again, none of this matters to an end user who just wants to #include
and go. You can do exactly this with Outcome. It neither uses nor
interferes with anything in Boost. It is an excellent neighbour,
including to other versions of itself.

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

Just because you don't understand the value of exception_ptr and nested
exceptions doesn't mean you are in a majority. Nested exceptions support
in the C++ 11 STL wouldn't be there unless WG21 thought them highly useful.

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

There are static asserts which fire if the types used don't meet their
requirements.

You are correct they are not documented in the reference API docs. There
is an open issue to fix that.

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

As I have said many times now, end users can #include and go. They don't
care, nor need to care, how the implementation works so long as it
cannot cause them misoperation.

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

I have seen completely uninitiated users get up and running with Outcome
in less than five minutes. If the end user has programmed in Rust or
Swift before, they just need to be told:

#include "boost.outcome/include/boost/outcome.hpp"

boost::outcome::expected<T, E>

... and they're good to go.

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

Thanks for the review.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/