Re: [review][LEAF] Review period ending soon - May 31

From: Stian Zeljko Vrba (vrba_at_[hidden])
Date: 2020-05-29 06:54:19

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 <boost-users-bounces_at_[hidden]> on behalf of Michael Caisse via Boost-users <boost-users_at_[hidden]>
Sent: Friday, May 29, 2020 12:02 AM
To: Boost users list <boost-users_at_[hidden]>
Cc: Michael Caisse <mcaisse-lists_at_[hidden]>
Subject: [Boost-users] [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
_______________________________________________
Boost-users mailing list
Boost-users_at_[hidden]
https://lists.boost.org/mailman/listinfo.cgi/boost-users