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

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 more convenient and safe *- Provides high-quality implementation of proposed std::expected (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 (an implementation provided in Outcome) is a generalization of std::optional<T> that provides a value, where the unhappy result is a 'std::error_code' or an instance of "your-chosen-error-type".

The library further provides 'outcome' for handling 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