On 2020-05-22 11:31, Michael Caisse via Boost wrote:

Please provide in your review information you think is valuable to understand your choice to ACCEPT or REJECT including LEAF as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).

I vote to conditionally accept LEAF into Boost. LEAF provides a new perspective on error propagation that is worth exploring in the real world. The conditions for acceptance are: 1. Provide a lower-level API for error handling which does not necessitate a pattern matching framework. I would prefer an API closer to the variant::index() and std::get<I>() combination that enables you to write the error handling as switch statements. More rationale for this request is given under the design evaluation.

- What is your evaluation of the design?

LEAF propagates error objects across multiple function calls by letting the callee store the error payload directly in a stack object, called the error context, at the caller. A pointer to the error context is located in thread-local storage. When there are more error contexts on the call stack, they are daisy chained via pointers in the child error context. This is a sound design, which gives LEAF an efficient way of propagating error payloads then a failure occurs. I like that the returned error_id can be passed through C code. There is runtime overhead in creating and destroying the error contexts even when no failure occurs. Whether or not that overhead is acceptable depends on circumstances. It seems to me that the noexcept performance overhead in the successful case and the failure case are not that different (assuming the error payload is not too heavy), so it may be a good idea to choose LEAF when worst-case performance is more important than average-case performance. The more I experimented with LEAF, the more I wished that there was an error handing API with less syntatic sugar. 1. Single-stepping into a TryBlock is really cumbersome. The developer has to single-step through layers of unfamiliar templated code before reaching their own TryBlock. This interrrupts their flow. Setting a breakpoint in the TryBlock helps but is still cumbersome. 2. Some compilation errors are difficult to interpret. For example, if you accidentally omit a required return statement in one of the try_handle_all handlers, then the compiler reports the start of the try_handle_all statement and some internal LEAF voodoo. If you are experienced with reading templates errors, then you may be able to discover the erroneous handler from the LEAF template parameters in the compiler output. The LEAF handling is reminiscient of a variant visitor, but with the variants we can use index() and std::get<T> in a switch statement instead of visitation. I would like to see something similar for LEAF error contexts. I am undecided about the reliance on thread-local storage. Initially I thought that we could just replace thread-local storage with a home- made lookup table using atomics (which are generally available on OS-less platforms, whereas mutexes etc are not,) but Niall's mention of consteval made me unsure (damn you Niall ;-) However, I do not see this as a show-stopper. The try_handle_all() and try_handle_some() functions have different return values. While I understand the rationale given in the reference documentation, it may still be confusing to users if they change from one to the other. This could possibly be solved by making it more clear in the documentation. The remote_handle_all() name is confusing. I would rename it to nested_handle_all().

- What is your evaluation of the implementation?

The implementation is of high quality, and reasonably readable if you are comfortable with templates. I did not do a proper code review, so the following are random notes. Some classes (leaf::result and the internal optional) uses placement-new and explicit destructor calls. I suggest that you create your own construct_at() and destruct_at() which uses the std counterparts when available, and placement-new / explicit destruction otherwise. This will make the code more readable. std::ostream is entangled into various classes for printing. I would prefer if printing was moved into seperate X_io.hpp files to speed up compilation times when not using them. The LEAF_CONSTEXPR should be renamed to LEAF_CXX14_CONSTEXPR to align with the BOOST_CXX14_CONSTEXPR naming. Also the #if should use '>=' instead of '>'. config.h includes . This should be replaced with Boost.WinAPI.

- What is your evaluation of the documentation?

Overall the documentation is well-presented. The examples reconfigures std::cout to throw exceptions. That obscures the examples because now the reader has to learn LEAF and how std::cout exceptions work at the same time. It would be better to move the std::cout exception stuff into a separate example which is only about std::cout.

- What is your evaluation of the potential usefulness of the library?

LEAF presents a unique error handling solution, that may be useful to certain users. I do not expect a huge user base though, as LEAF users mainly are going to be projects with requirements not met by the alternatives.

- Did you try to use the library? With which compiler(s)? Did you have any problems?

I experimented with small examples and found no compiler-related issues.

- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

About 6 hours. Spent most time trying understanding the design than reviewing the code and documentation.

- Are you knowledgeable about the problem domain?

Yes.