[review][LEAF] Review period ending soon - May 31
I'd like to encourage each of you to review Emil's error handling library. We often focus on design or implementation based reviews, and I encourage those but what I would really like to see are more user based reviews. Grab the library and docs and play with it a bit. What works well? What can be improved? As a reminder, some of LEAF's features include: * arbitrary error types * header-only * no dependencies * use without exceptions * use with exceptions * no dynamic memory alloc You can find the documentation here: https://zajo.github.io/leaf/ and the library source here: https://github.com/zajo/leaf/tree/review Thanks! michael -- Michael Caisse Ciere Consulting ciere.com
My informal, "stream of thought" review, written during linearly reading the documentation. The documentation begins with a five-minute tutorial, excellent so far! Then I start reading the example code and the first thing that strikes me: `try_handle_all` takes a list of lambdas. Oo. Looks like unmaintainable mess in the long run. Inside a class, would it be possible to pass in list of method pointers (e.g., `try_handle_all(this, &Class::Try, &Class::ErrorHandler1, &Class::ErrorHandler2)`? The second thing that strikes me is that each error handler takes a `leaf::match` that is a variadic template, but what are its valid parameters? Also, the lambda itself takes a different number parameters, and the relation between what is inside `leaf::match<...>` and the rest of lambda parameters is totally non-obvious. So... ok, bad for learnability, it seems I have to check the docs every single time I want to handle an error. So the comments below the code explain what is happening, like "This handler will be called if the detected error includes". Now a questions arises: how does the detected error "include" any information? Scrolling down to `file_open` function that generates `input_file_open_error`: ok, it returns an error object with errno. But what happened to the *filename* that is expected by both handlers for `input_file_open_error` in main? And since the explanation of error handler says "[...] if the detected error includes: [...] AND an object of type `leaf::e_file_name`" -- does this imply the error handler won't match since the file name is missing? Having to "tell" LEAF that an enum is error code by directly poking into its "implementation" namespace feels "dirty". Yes, I know, that's how C++ works, even `std::` has "customization points", but it still feels "dirty". Then, macros, `LEAF_AUTO` and `LEAF_CHECK`. This doesn't belong to modern C++. (Indeed, how will it play with upcoming modules?) Another question arises: how in the world am I going to debug this? If I "step into" `try_handle_all` how many levels of internal stack traces do I have to traverse to get to the try lambda? Similarly, when bailing out to a handler? In debugger, how do I inspect an error that's not matched by any handler? Then on to exception handling: same lambda-mess. Then one encounters `leaf::preload` and `leaf::defer`. Ok... all exceptions will somehow get filename "associated" with it beyond this point. Magic behind the scenes, but ok. A question in my mind pops up: what happens if the method passed to `defer` throws an exception? We're talking about error-handling, so this should be well-defined. (Oh, ok, it's mentioned in the reference for defer: executed from dtor, so expect your program to terminate, and there's no safety net.) So exception handlers: `leaf::catch_`. The same questions/problems apply as for matching. We have `input_file_open_error`, the lambda expects a file name, but the file name is nowhere supplied when throwing an error. Oh, wait, that's the purpose of `preload` I guess. I overlooked that one with the sample code using `leaf::match`. Then on to tutorial. A bunch of definitions, error_id being the most conspicuous one: "program-wide unique identifier of a particular occurrence of failure". Hmm, how's that going to work with plain throw statements that do not use leaf::exception? OK, somewhat explained under "Interoperability" heading. Some boiler-plate code needed for integrating with libraries not designed around LEAF (i.e.... most?) At this point, I stopped reading. ---
From what I've read, I don't like it and I can't see myself using it: Overall, it feels as if it's heavily biased towards error-handling based on return values, exceptions being a 2nd-class citizen (re "Integration" section). Also, it's half-declarative (static pattern matching) and half-dynamic (`leaf::preload/defer`). Whether an error handler executes depends on whether the error-configuration part of the code (preload/defer) has executed. Does not bode well for robust error handling.
As for whether it should be accepted into Boost: "weak no", because it's not general-purpose enough. From reading the "Interoperability" section once again, LEAF seems to be most comfortably used with other code designed around LEAF.
________________________________
From: Boost-users
On Thu, May 28, 2020 at 11:55 PM Stian Zeljko Vrba via Boost-users < boost-users@lists.boost.org> wrote:
Then I start reading the example code and the first thing that strikes me: `try_handle_all` takes a list of lambdas. Oo. Looks like unmaintainable mess in the long run. Inside a class, would it be possible to pass in list of method pointers (e.g., `try_handle_all(this, &Class::Try, &Class::ErrorHandler1, &Class::ErrorHandler2)`?
The second thing that strikes me is that each error handler takes a `leaf::match` that is a variadic template, but what are its valid
This is a lot like when you use try, it is followed by a list of catch
statements. I wouldn't call that unmaintainable.
parameters? Also, the lambda itself takes a different number parameters,
and the relation between what is inside `leaf::match<...>` and the rest of
lambda parameters is totally non-obvious. So... ok, bad for learnability,
it seems I have to check the docs every single time I want to handle an
error.
Yes it helps to read the docs. You don't have to use match, it's just a way
to select a subset of enumerated values automatically. If you want to write
a handler for some enum type my_error_code, you could just say:
[]( my_error_code ec )
{
switch(ec)
{
case err1:
case err2:
.... // Handle err1 or err2 errors
case err5:
.... // Handle err5 errors
}
}
But you could instead use match to split it into two handlers:
[]( leaf::match
Having to "tell" LEAF that an enum is error code by directly poking into its "implementation" namespace feels "dirty". Yes, I know, that's how C++ works, even `std::` has "customization points", but it still feels "dirty".
This is done to protect the user from accidentally passing to LEAF some random type as an error type (since LEAF can take pretty much any movable object). I'm considering removing is_e_type.
Then, macros, `LEAF_AUTO` and `LEAF_CHECK`. This doesn't belong to modern C++. (Indeed, how will it play with upcoming modules?)
So exception handlers: `leaf::catch_`. The same questions/problems apply as for matching. We have `input_file_open_error`, the lambda expects a file name, but the file name is nowhere supplied when throwing an error. Oh, wait, that's the purpose of `preload` I guess. I overlooked that one with
You don't have to use the helper macros. They're typical for any error handling library that can work without exceptions. You don't need them if you use exceptions. the sample code using `leaf::match`. Helps to read the docs.
From what I've read, I don't like it and I can't see myself using it: Overall, it feels as if it's heavily biased towards error-handling based on return values, exceptions being a 2nd-class citizen.
The library is neutral towards exceptions vs. error codes, if anything it lets you deal with return values as easily as with exceptions. In my own code I prefer exceptions, so most definitely not 2nd class.
Whether an error handler executes depends on whether the error-configuration part of the code (preload/defer) has executed. Does not bode well for robust error handling.
You can write the code so the same error handler is matched regardless of
whether e.g. e_file_name is available:
[]( catch_
Thanks for additional explanations.
This is a lot like when you use try, it is followed by a list of catch statements. I wouldn't call that unmaintainable.
Ok, a question from my original email that you did not address : is `try_handle_all(this, &Class::Try, &Class::Handler1, &Class::Handler2)` possible at all? [I should note here that I have a rather low threshold for creating a class: whenever a group of functions operate on some "common context", I put them into a class. I prefer this to passing the "context" between them as arguments, even if the individual functions -- technically -- could be used standalone. Experience shows that, in application code, they almost never are.] I admit that "unmaintainable mess" was too vague/harsh; it was a visceral reaction of the kind "I don't want to look at this kind of code daily". The phrase consists of two parts I haven't had the immediate insight to break down like this: * First part: the lambda-based API introduces definitely more syntactic noise compared to to try/catch blocks, hence why the above question (about method pointers) was the question I IMMEDIATELY thought of when encountering the very first example. It doesn't solve the second part though. * Second part: exception catching is only based on polymorphic matching on the exception type. When I see `catch (SomeException& e)` I know immediately what it handles. When I see a LEAF's lambda-handler, I first have to parse the syntactic noise, then I have to extract error code(s) or exception(s), then I have to extract additional parameters (these determine whether a handler is called),... A lot of cognitive overhead to determine when the handler runs, especially when the answer can depend on the run-time history of the program (preload/defer). Other questions that I thought of since yesterday. Looking at exception example, can `leaf::catch_` take a list of exceptions (match can take a list of error codes)? If yes, it'd be a nice upgrade to ordinary catch blocks. How does `preload/defer` interact with `new_error`? Concretely, in the 5-minute intro: you use `preload` to "fill in" the file name. What would happen if `new_error` in `file_open` ALSO specified a file name as additional data? Would it override what was preloaded or would it get ignored?
You could write a handler that takes all pointer arguments and then it would match all errors and you can do your own logic.
"I, the code writer" could, yes.
When shifting my perspective to "I, the chief engineer and code reviewer", I would not like to review LEAF-based error handling because it is non-obvious when the error handler runs. Even if I banned the use of "dynamic" features on a project (e.g., preload/defer), the answer is STILL dependent on subtleties like handler taking additional info by pointer or reference, and error-generating methods supplying (or not) the relevant "additional information" for the handlers.
________________________________
From: Boost-users
Then I start reading the example code and the first thing that strikes me: `try_handle_all` takes a list of lambdas. Oo. Looks like unmaintainable mess in the long run. Inside a class, would it be possible to pass in list of method pointers (e.g., `try_handle_all(this, &Class::Try, &Class::ErrorHandler1, &Class::ErrorHandler2)`?
This is a lot like when you use try, it is followed by a list of catch statements. I wouldn't call that unmaintainable.
The second thing that strikes me is that each error handler takes a `leaf::match` that is a variadic template, but what are its valid parameters? Also, the lambda itself takes a different number parameters, and the relation between what is inside `leaf::match<...>` and the rest of lambda parameters is totally non-obvious. So... ok, bad for learnability, it seems I have to check the docs every single time I want to handle an error.
Yes it helps to read the docs. You don't have to use match, it's just a way to select a subset of enumerated values automatically. If you want to write a handler for some enum type my_error_code, you could just say:
[]( my_error_code ec )
{
switch(ec)
{
case err1:
case err2:
.... // Handle err1 or err2 errors
case err5:
.... // Handle err5 errors
}
}
But you could instead use match to split it into two handlers:
[]( leaf::match
Having to "tell" LEAF that an enum is error code by directly poking into its "implementation" namespace feels "dirty". Yes, I know, that's how C++ works, even `std::` has "customization points", but it still feels "dirty".
This is done to protect the user from accidentally passing to LEAF some random type as an error type (since LEAF can take pretty much any movable object). I'm considering removing is_e_type.
Then, macros, `LEAF_AUTO` and `LEAF_CHECK`. This doesn't belong to modern C++. (Indeed, how will it play with upcoming modules?)
You don't have to use the helper macros. They're typical for any error handling library that can work without exceptions. You don't need them if you use exceptions.
So exception handlers: `leaf::catch_`. The same questions/problems apply as for matching. We have `input_file_open_error`, the lambda expects a file name, but the file name is nowhere supplied when throwing an error. Oh, wait, that's the purpose of `preload` I guess. I overlooked that one with the sample code using `leaf::match`.
Helps to read the docs.
From what I've read, I don't like it and I can't see myself using it: Overall, it feels as if it's heavily biased towards error-handling based on return values, exceptions being a 2nd-class citizen.
The library is neutral towards exceptions vs. error codes, if anything it lets you deal with return values as easily as with exceptions. In my own code I prefer exceptions, so most definitely not 2nd class.
Whether an error handler executes depends on whether the error-configuration part of the code (preload/defer) has executed. Does not bode well for robust error handling.
You can write the code so the same error handler is matched regardless of whether e.g. e_file_name is available:
[]( catch_
I should explain myself a bit more:
When shifting my perspective to "I, the chief engineer and code reviewer", I would not like to review LEAF-based error handling because it is non-obvious when the error handler runs.
In a way, it IS obvious when the error handler runs: when there is a match AND when all "additional information" required by the lambda is present, unless received by a pointer in which case absence is passed as null. What is NOT obvious in each error handler is when "additional data" is present as it depends on run-time history.
IMHO, execution of error handler should depend only on whether an error condition occurred, not on presence of "additional data" supplied by the application at run-time. Taking a handler from the 5-min intro:
[](leaf::match
This is a lot like when you use try, it is followed by a list of catch statements. I wouldn't call that unmaintainable.
Ok, a question from my original email that you did not address : is `try_handle_all(this, &Class::Try, &Class::Handler1, &Class::Handler2)` possible at all? [I should note here that I have a rather low threshold for creating a class: whenever a group of functions operate on some "common context", I put them into a class. I prefer this to passing the "context" between them as arguments, even if the individual functions -- technically -- could be used standalone. Experience shows that, in application code, they almost never are.] I admit that "unmaintainable mess" was too vague/harsh; it was a visceral reaction of the kind "I don't want to look at this kind of code daily". The phrase consists of two parts I haven't had the immediate insight to break down like this: * First part: the lambda-based API introduces definitely more syntactic noise compared to to try/catch blocks, hence why the above question (about method pointers) was the question I IMMEDIATELY thought of when encountering the very first example. It doesn't solve the second part though. * Second part: exception catching is only based on polymorphic matching on the exception type. When I see `catch (SomeException& e)` I know immediately what it handles. When I see a LEAF's lambda-handler, I first have to parse the syntactic noise, then I have to extract error code(s) or exception(s), then I have to extract additional parameters (these determine whether a handler is called),... A lot of cognitive overhead to determine when the handler runs, especially when the answer can depend on the run-time history of the program (preload/defer). Other questions that I thought of since yesterday. Looking at exception example, can `leaf::catch_` take a list of exceptions (match can take a list of error codes)? If yes, it'd be a nice upgrade to ordinary catch blocks. How does `preload/defer` interact with `new_error`? Concretely, in the 5-minute intro: you use `preload` to "fill in" the file name. What would happen if `new_error` in `file_open` ALSO specified a file name as additional data? Would it override what was preloaded or would it get ignored?
You could write a handler that takes all pointer arguments and then it would match all errors and you can do your own logic.
"I, the code writer" could, yes.
When shifting my perspective to "I, the chief engineer and code reviewer", I would not like to review LEAF-based error handling because it is non-obvious when the error handler runs. Even if I banned the use of "dynamic" features on a project (e.g., preload/defer), the answer is STILL dependent on subtleties like handler taking additional info by pointer or reference, and error-generating methods supplying (or not) the relevant "additional information" for the handlers.
________________________________
From: Boost-users
Then I start reading the example code and the first thing that strikes me: `try_handle_all` takes a list of lambdas. Oo. Looks like unmaintainable mess in the long run. Inside a class, would it be possible to pass in list of method pointers (e.g., `try_handle_all(this, &Class::Try, &Class::ErrorHandler1, &Class::ErrorHandler2)`?
This is a lot like when you use try, it is followed by a list of catch statements. I wouldn't call that unmaintainable.
The second thing that strikes me is that each error handler takes a `leaf::match` that is a variadic template, but what are its valid parameters? Also, the lambda itself takes a different number parameters, and the relation between what is inside `leaf::match<...>` and the rest of lambda parameters is totally non-obvious. So... ok, bad for learnability, it seems I have to check the docs every single time I want to handle an error.
Yes it helps to read the docs. You don't have to use match, it's just a way to select a subset of enumerated values automatically. If you want to write a handler for some enum type my_error_code, you could just say:
[]( my_error_code ec )
{
switch(ec)
{
case err1:
case err2:
.... // Handle err1 or err2 errors
case err5:
.... // Handle err5 errors
}
}
But you could instead use match to split it into two handlers:
[]( leaf::match
Having to "tell" LEAF that an enum is error code by directly poking into its "implementation" namespace feels "dirty". Yes, I know, that's how C++ works, even `std::` has "customization points", but it still feels "dirty".
This is done to protect the user from accidentally passing to LEAF some random type as an error type (since LEAF can take pretty much any movable object). I'm considering removing is_e_type.
Then, macros, `LEAF_AUTO` and `LEAF_CHECK`. This doesn't belong to modern C++. (Indeed, how will it play with upcoming modules?)
You don't have to use the helper macros. They're typical for any error handling library that can work without exceptions. You don't need them if you use exceptions.
So exception handlers: `leaf::catch_`. The same questions/problems apply as for matching. We have `input_file_open_error`, the lambda expects a file name, but the file name is nowhere supplied when throwing an error. Oh, wait, that's the purpose of `preload` I guess. I overlooked that one with the sample code using `leaf::match`.
Helps to read the docs.
From what I've read, I don't like it and I can't see myself using it: Overall, it feels as if it's heavily biased towards error-handling based on return values, exceptions being a 2nd-class citizen.
The library is neutral towards exceptions vs. error codes, if anything it lets you deal with return values as easily as with exceptions. In my own code I prefer exceptions, so most definitely not 2nd class.
Whether an error handler executes depends on whether the error-configuration part of the code (preload/defer) has executed. Does not bode well for robust error handling.
You can write the code so the same error handler is matched regardless of whether e.g. e_file_name is available:
[]( catch_
On Thu, May 28, 2020 at 11:54 PM Stian Zeljko Vrba via Boost-users
My informal, "stream of thought" review, written during linearly reading the documentation.
Wonderful review, written in a down to earth style which succinctly captures some of my own thoughts, thank you.
Then I start reading the example code and the first thing that strikes me: `try_handle_all` takes a list of lambdas. Oo. Looks like unmaintainable mess in the long run.
If you have a moment I would love to hear your thoughts on this style of code, which seems to share some of the same interface styles as LEAF: https://github.com/facebookexperimental/libunifex/blob/8311d141d6654acbff269... This is the direction that wg21 is going in with respect to asynchrony. Thanks
Wonderful review, written in a down to earth style which succinctly captures some of my own thoughts, thank you.
Thanks.
If you have a moment I would love to hear your thoughts on this style of code, which seems to share some of the same interface styles as LEAF:
Even worse than lambda-style of LEAF. With lambdas, I can at least use IDE/editor to collapse individual lambda blocks and get some overview of the structure, but editor features are of no use for this lisp-ish style of nested function invocations. I skimmed over the code, and when I got to `get_stop_token` and `stopWrite...` (the last two lines before catching system error) arguments, I have no idea which function they belong to.
Maybe it would be somewhat more appealing if inline lambdas were own functions.
Even without knowing what all these functions do, it seems extremely unintuitive having to wrap lambdas into `lazy` and `defer`. (And what's the difference, their definitions are totally unilluminating.)
Also, how is this supposed to be debugged? How are the error messages when you mess something up?
This looks nightmare-ish, way worse than LEAF. The basic structure of C++ is block-based, this abuses function invocation notation to the point of almost forcing the user into writing LISP.
________________________________
From: Vinnie Falco
My informal, "stream of thought" review, written during linearly reading the documentation.
Wonderful review, written in a down to earth style which succinctly captures some of my own thoughts, thank you.
Then I start reading the example code and the first thing that strikes me: `try_handle_all` takes a list of lambdas. Oo. Looks like unmaintainable mess in the long run.
If you have a moment I would love to hear your thoughts on this style of code, which seems to share some of the same interface styles as LEAF: https://github.com/facebookexperimental/libunifex/blob/8311d141d6654acbff269... This is the direction that wg21 is going in with respect to asynchrony. Thanks
Here is my non-review of LEAF. As far as I am concerned, the only reason for LEAF to exist at all is performance. Code that uses LEAF contains more syntactic noise than code than the equivalent code using Boost.Exception, and is harder to debug. The complex pattern matching aspect of LEAF seems like it could occasionally be useful, but not not often enough to justify its cognitive overhead. Almost all of my error handlers fall into one of three categories: - Try something else. - Log the error, possibly show a message box, then ignore it. - Log the error, possibly show a message box, then terminate. In none of these cases do I need any extra error information beyond the basic type of the error, except for the purpose of converting the error to a user-readable string that is then logged and possibly to the user through a message box. So, performance. Apparently some people use result<>-style return values for performance reasons, because they cannot afford exceptions. Apparently LEAF avoids dynamic memory when used with exceptions where Boost.Exception does not, which all else being equal, should improve performance. But LEAF has its own complex logic under the hood that could have its own performance problems. In theory, there is no reason why an ideal optimizer should create worse binary code for a program using Boost.Exception than one using LEAF, with or without exceptions. In practice, it could well be that LEAF (especially LEAF with result<>) is much faster, but I have seen no evidence of this. Nor do I consider myself competent to write a benchmark to test this. I know enough about writing benchmarks to know how easy it is to get them wrong, but not enough to avoid doing so. So, since I am unable to determine whether what I consider the key feature of LEAF is actually working as promised or not, I cannot in good conscience vote to either accept or reject LEAF. I realize that I am holding LEAF to a very high standard here. This is because the problem that LEAF is trying to solve is so ubiquitous, and the consequences of using LEAF are so far-reaching. It makes no sense for a unified program to use LEAF in some subsystems but not in others. I have to either embrace LEAF entirely or reject it entirely, and I don't have enough information to make an informed choice. -- Rainer Deyke (rainerd@eldwood.com)
On Sat, May 30, 2020 at 7:36 AM Rainer Deyke via Boost-users < boost-users@lists.boost.org> wrote:
I realize that I am holding LEAF to a very high standard here. This is because the problem that LEAF is trying to solve is so ubiquitous, and the consequences of using LEAF are so far-reaching. It makes no sense for a unified program to use LEAF in some subsystems but not in others.
Thank you for the non-review. :) Actually, LEAF is well suited to be deployed only when needed. It provides a type-safe channel that can be used at any point in your program to communicate with error handlers. This channel is available even in tricky use cases, for example when error objects need to be communicated out of a C callback or through another uncooperative interface that is beyond your control. This fits C++ well, because we tend to use a lot of generic intermediate functions which, hand-waving from other error-handling library authors notwithstanding, will use all kinds of different types to report errors.
participants (5)
-
Emil Dotchevski -
Michael Caisse -
Rainer Deyke -
Stian Zeljko Vrba -
Vinnie Falco