[boost] [review] [Outcome] Deniz' review

Subject: [boost] [review] [Outcome] Deniz' review
From: Deniz Bahadir (dbahadir_at_[hidden])
Date: 2017-05-24 11:33:49

Hi everyone,

this is my review of Niall's Outcome library.

It is my fist review, so better keep that in mind when considering my 
answers for Outcome's acceptance into Boost.

> The formal review of Niall Douglas' Outcome library starts today
> (Fri-19-May) and continues until Sun-28-May.
> 
> Your participation is encouraged, as the proposed library is uncoupled and
> focused, and reviewers don't need to be domain experts to appreciate the
> potential usefulness of the library and to propose improvements.  Everyone
> needs (and has suffered) error handling, and can compose an opinion on that
> topic.
> 
> Outcome is a header-only C++14 library providing expressive and type-safe
> ultra-lightweight error handling, suitable for low-latency code bases.
> 
I think it is a good decision to target a more modern C++ standard, 
especially if it eases the implementation. (I am not such a big fan of 
re-implementing what is already in a particular standard, just to 
support old compilers. At least not for a new library.)

> Key features:
> 
> *- Makes using std::error_code from C++11's <system_error> more convenient
> and safe
> *- Provides high-quality implementation of proposed std::expected<T,E> (on
> C++20 standards track)
> *- Good focus on low-latency (with tests and benchmarks)
> *- Error-handling algorithmic composition with-or-without C++ exceptions
> enabled
> *- No dependencies (not even on Boost)
> 
The last point, I think, is not really bad.

Even if Outcome will be accepted into Boost and therefore becomes a part 
of Boost I think it is good idea to only depend on other Boost modules 
if it is absolutely necessary. (For example, I would not consider anyone 
re-inventing Boost.Hana. But if a simple Tribool class is needed, I am 
all for providing a simple one, especially, if it is just an 
implementation detail.)

And as the library targets C++14 I am all for using smart-pointers from 
the standard instead of having a dependency on Boost.SmartPtr or 
Boost.Move. etc.

> 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.
> 
> [...]
> 
> Here are some
> questions you might want to answer in your review:
> 
> - What is your evaluation of the design?
> 
I like it. The idea of combining different error-/return-code and 
exception handling seems great to me.
Ensuring, that it can be compiled into very efficient machine-code is a 
very nice plus.

However, I would like Niall to recheck if several of his 
code-optimizations that make the code hard to understand or maintain are 
still really needed for the set of supported compilers. As was mentioned 
in previous email-discussions this might not always be the case.
(Maybe, it is even possible instead to provide some hints to 
compiler-vendors where to try to improve their optimizers?)

> - What is your evaluation of the implementation?
> 
It fascinates me to see so many macros in headers which auto-generate 
the names of header-files to be included. I personally like such 
automatisms where one only has to modify the values of some few macros 
in a single file to get a totally different behavior or implementation. 
(I myself happen to implement such automatisms quite often.)

However, that always carries the risk of confusing the user/reviewer (or 
the future maintainer) who tries to understand what is going on. In 
general, this might be solvable with even more documentation/comments, 
but is still more work to grasp what's going on. However, a general user 
might not care at all as long as everything works fine out of the box. 
(So let's hope it always does.)

Other than that, the implementation looks solid. Although, I must 
confess that I did not look into it in-depth.
(It at least seems to be better documented with comments than many other 
Boost-libraries that I have looked into and tried to understand what is 
going on.)

> - What is your evaluation of the documentation?
> 
The "tutorial" is quite long but also quite good. I think I understood 
everything, because it explains most things in great detail. That's, why 
I would not consider it a "tutorial" but an in-depth documentation.

I would expect a tutorial being more of a short introduction which 
explains the basic usage in not more than 3 to 4 screen-pages.
The "landing-page" is more what I would have thought a tutorial would 
look like; a little longer maybe with smaller examples and some more 
text between them.

But then again, that could just be me having a wrong impression what a 
tutorial in general should look like.
At least, the entire documentation (and also the link to the ACCU-talk) 
helped me to understand what Outcome is for.

> - What is your evaluation of the potential usefulness of the library?
> 
I think, it is very useful.

I myself would probably just need the main functionality "expected" 
provides (value or error) but having some extensions which can handle 
exceptions, too, is a good thing.

With "outcome" and "result" being possibly empty I am not so sure. It 
probably is really better to report the empty-case through the 
error-code. (But I think, Niall is already considering this.)

The benefit of having "option" is not totally clear to me. It looks 
similar to "std::optional". Is its implementation just more efficient 
than the one found in different standard-libraries? (Then again, 
std::optional comes with C++17 and Outcome targets C++14.) How about 
Boost.Optional? Would it make sense to have a dependency on it instead 
(and possibly improve its implementation with some tricks of Outcome's 
"option")?

If "outcome" and "result" report the empty-case through the error-code 
then that would make "option" obsolete, because one could use "result" 
instead with an error_code that will be set to empty_value (or how it 
would be called). That would also remove a possible dependency on 
Boost.Optional.

> - Did you try to use the library? With what compiler? Did you have any
> problems?
> 
No, sorry, I did not.

> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
> 
Several hours reading the docs, watching the ACCU-talk (and reading the 
mail-discussions on this mailing-list). A quick look into the code.

> - Are you knowledgeable about the problem domain?
> 
Just as much as a developer could be who has worked with code that uses 
exceptions, error-codes, return-codes and has lazy-co-developers (me 
included) that forget to check the return-codes or to catch exceptions.

> And most importantly:
> 
> - Do you think the library should be accepted as a Boost library?
> 
Yes, I would say so.
This would be an unconditional yes, even though I would suggest to add a 
small "introductory-tutorial" to the documentation and would like to 
hear what Niall (and others) think about my above remarks concerning 
removal of "option" and re-checking of the specific compiler-optimizations.

> For more information about Boost Formal Review Process, see:
> http://www.boost.org/community/reviews.html
> 
> Thank you very much for your time and efforts.
> 
> --charley

Thank you very much for putting so much work into Outcome, Niall, and 
Charley for being its review-manager.

Deniz Bahadir