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

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

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