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

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

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

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

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