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