Re: [boost] [review][LEAF] Review of LEAF starts today : May 22 - May 31
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.
On Sat, May 30, 2020 at 11:18 AM Bjorn Reese via Boost < boost@lists.boost.org> wrote:
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.
I wrote a benchmark. It's worth mentioning that when used without exception handling, LEAF does not depend on aggressive RVO optimizations to be able to communicate errors efficiently, since they are not contained in the return object. They skip the stack frames and go straight to the error handler.
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.
Yes. You can set breakpoints at these two lines to stop just before an error handler is called: https://github.com/zajo/leaf/blob/develop/include/boost/leaf/handle_error.hp... https://github.com/zajo/leaf/blob/develop/include/boost/leaf/handle_error.hp...
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.
Also yes. I'm all ears if you have suggestions. This stuff is tricky, e.g. consider that when using try_handle_some, if you're returning a result<void>, it would be unfortunate to require a return { } from the error handlers (in case of success). LEAF does not require it, but there's code to deal with that and maybe a few other similar voodoo.
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.
The difficulty with that is, context<E...> requires a list of types so it knows what to put in the tuple. In other words, before you are able to say ctx.get<T>(), you must have included that T in the types used to instantiate context<E...>, and if you didn't, you'd always get a null. Worse, if you missed to include a T in the list of types, and an error-reporting function attempts to communicate a T, it'll get discarded (no storage for it). That's why LEAF deduces the E... type list from the types that error handlers take as arguments. This way, if a T is discarded, it means no error handlers needed it, and so it'd be wasteful to spend cycles transporting it.
The remote_handle_all() name is confusing. I would rename it to nested_handle_all().
That would be misleading because it may not be nested (English is not my first language). I've spent a long time trying to come up with this name, I do want to rename it if a better name is suggested.
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.
Not possible because printing is optional. LEAF will bind it if available, but if not, you won't get a compile error. This is the same as in Boost Exception.
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 '>'.
Yes, the constexpr stuff needs to be cleaned up and made perfectly correct.
- 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.
In a way it isn't so strange. It's as if catch() could take multiple arguments that are matched by their static, rather than dynamic type. Thanks once again!
On 2020-05-31 01:18, Emil Dotchevski via Boost wrote:
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.
The difficulty with that is, context<E...> requires a list of types so it knows what to put in the tuple. In other words, before you are able to say ctx.get<T>(), you must have included that T in the types used to instantiate context<E...>, and if you didn't, you'd always get a null. Worse, if you missed to include a T in the list of types, and an error-reporting function attempts to communicate a T, it'll get discarded (no storage for it). That's why LEAF deduces the E... type list from the types that error handlers take as arguments. This way, if a T is discarded, it means no error handlers needed it, and so it'd be wasteful to spend cycles transporting it.
I made a typo above. I meant to say std::get<I>. I am asking for a low-level API, not a bullet-proof API. If the latter is needed, then your existing pattern matching solution works just fine. The use of such a low-level API would look something like this: leaf::context<E0, E1> ctx; ctx.activate(); // I would like to avoid this though auto ret = foo(); if (!ret) { switch (ctx.index(ret.error())) { case 0: // E0 handle_E0(ctx.get<0>(ret.error()); break; case 1: // E1 handle_E1(ctx.get<1>(ret.error()); break; default: // Unknown error type panic(); break; } }
The use of such a low-level API would look something like this:
Hmm, I'm failing to see the overwhelming value in exposing these portions of the API. As of now, one does not simply get a single value of the error context. Instead, the context holds many different values and then applies them to a user-provided Callable. `try_handle_all` in essence does the `std::apply` dance for you. If one was to manually operate with the tuple themselves, they'd essentially re-implement what already exists in LEAF today. - Christian
On 31.05.20 18:03, Christian Mazakas via Boost wrote:
The use of such a low-level API would look something like this:
Hmm, I'm failing to see the overwhelming value in exposing these portions of the API.
Here is one use case for some kind of low-level access to error objects without going through lambdas (although it would have a different form than the one used by Bjorn). Suppose I have an error type that may or may not be annotated with a file name, an IP address, or a user name, individually or in any combination. For each of these annotations, I want to handle it the same whenever it is present, regardless of the others. Without some sort of direct access to error objects, the code for this would look something like this: return leaf::try_handle_some( some_try_block, [](leaf::match<my_error>, leaf::e_file_name fname, my::e_user_name uname, my::ip_address ip) { handle_filename(fname); handle_user_name(uname); handle_ip(ip); return handle_error(); }, [](leaf::match<my_error>, leaf::e_file_name fname, my::e_user_name uname) { handle_filename(fname); handle_user_name(uname); return handle_error(); }, [](leaf::match<my_error>, leaf::e_file_name fname, my::ip_address ip) { handle_filename(fname); handle_ip(ip); return handle_error(); }, [](leaf::match<my_error>, leaf::e_file_name fname) { handle_filename(fname); return handle_error(); }, [](leaf::match<my_error>, my::e_user_name uname, my::ip_address ip) { handle_user_name(uname); handle_ip(ip); return handle_error(); }, [](leaf::match<my_error>, my::e_user_name uname) { handle_user_name(uname); return handle_error(); }, [](leaf::match<my_error>, my::ip_address ip) { handle_ip(ip); return handle_error(); }, [](leaf::match<my_error>) { return handle_error(); }); That's a lot of redundant code that could be avoided if I had a way to query at runtime if a particular annotation is present or not. -- Rainer Deyke (rainerd@eldwood.com)
Rainer Deyke wrote:
Here is one use case for some kind of low-level access to error objects without going through lambdas (although it would have a different form than the one used by Bjorn). Suppose I have an error type that may or may not be annotated with a file name, an IP address, or a user name, individually or in any combination. For each of these annotations, I want to handle it the same whenever it is present, regardless of the others. Without some sort of direct access to error objects, the code for this would look something like this:
return leaf::try_handle_some( some_try_block, [](leaf::match<my_error>, leaf::e_file_name fname, ...
I think that you can match by pointer here:
leaf::e_file_name * fname,
which will give you nullptr if the file name isn't present.
On Sun, May 31, 2020 at 10:32 AM Rainer Deyke via Boost < boost@lists.boost.org> wrote:
On 31.05.20 18:03, Christian Mazakas via Boost wrote:
The use of such a low-level API would look something like this:
Hmm, I'm failing to see the overwhelming value in exposing these
portions
of the API.
Here is one use case for some kind of low-level access to error objects without going through lambdas (although it would have a different form than the one used by Bjorn). Suppose I have an error type that may or may not be annotated with a file name, an IP address, or a user name, individually or in any combination. For each of these annotations, I want to handle it the same whenever it is present, regardless of the others. Without some sort of direct access to error objects, the code for this would look something like this:
return leaf::try_handle_some( some_try_block, [](leaf::match<my_error>, leaf::e_file_name fname, my::e_user_name uname, my::ip_address ip) { handle_filename(fname); handle_user_name(uname); handle_ip(ip); return handle_error(); }, <deleted long list of lambdas
No, you'd simply do: return leaf::try_handle_some( some_try_block, [](leaf::match<my_error, my_error::something>, leaf::e_file_name const * fname, my::e_user_name const * uname, my::ip_address const * ip) { if( fname ) handle_filename(*fname); if( uname ) handle_user_name(*uname); if( ip ) handle_ip(*ip); return handle_error(); } ); When you take an argument by const & or by value, LEAF does the ifs for you before calling your function (once it determines that all of its arguments are available). If an argument is taken by const *, and the value is not available, LEAF will still call the function, passing null for that argument.
When you take an argument by const & or by value, LEAF does the ifs for you before calling your function (once it determines that all of its arguments are available). If an argument is taken by const *, and the value is not available, LEAF will still call the function, passing null for that argument.
Given the discussion I feel this is something that should appear somewhere near the top of the tutorial
On Sun, May 31, 2020 at 9:03 AM Christian Mazakas via Boost < boost@lists.boost.org> wrote:
The use of such a low-level API would look something like this:
Hmm, I'm failing to see the overwhelming value in exposing these portions of the API.
One possible use would be to print (for diagnostic reasons) all the data available in the context, but that is supported directly: []( leaf::diagnostic_info const & info ) { std::cout << info; } That said, it is easy to provide the functionality Bjorn asked for. It'd be nice to see a use case though, I've never needed it.
On 2020-05-31 20:27, Emil Dotchevski via Boost wrote:
That said, it is easy to provide the functionality Bjorn asked for. It'd be nice to see a use case though, I've never needed it.
There are no additional functional use cases per se, but am primarily coming from the noexcept use cases on embedded devices, where I am more interested in an inexpensive, non-allocating error propagation mechanism than modelling exception-like structures. The index/get API is intended to address various concerns with the pattern matching API. The advantages are: Single-step debugging becomes easy. Compiler error-messages becomes more readable. Code becomes easier to review because it relies on familiar C++ constructs such as if- and switch-statements. Faster performance of normal path. I wrote a quick implementation of get<I>() based on peek<T>() as you suggested, and index() was implemented as a recursive search through the tuple for the error_id. My benchmark shows that the happy-path is faster with index/get than with try_handle_all().
On Sun, May 31, 2020 at 6:51 AM Bjorn Reese via Boost <boost@lists.boost.org> wrote:
I made a typo above. I meant to say std::get<I>.
I am asking for a low-level API, not a bullet-proof API. If the latter is needed, then your existing pattern matching solution works just fine.
The use of such a low-level API would look something like this:
leaf::context<E0, E1> ctx; ctx.activate(); // I would like to avoid this though
auto ret = foo(); if (!ret) { switch (ctx.index(ret.error())) { case 0: // E0 handle_E0(ctx.get<0>(ret.error()); break;
case 1: // E1 handle_E1(ctx.get<1>(ret.error()); break;
default: // Unknown error type panic(); break; } }
I think you're essentially asking for access to this function, through slightly different interface: https://github.com/zajo/leaf/blob/develop/include/boost/leaf/handle_error.hp... .
participants (6)
-
Alexander Grund -
Bjorn Reese -
Christian Mazakas -
Emil Dotchevski -
Peter Dimov -
Rainer Deyke