Re: [review][LEAF] Review of LEAF starts today : May 22 - May 31

From: Bjorn Reese (breese_at_[hidden])
Date: 2020-05-30 18:17:47

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