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<error_code, input_file_open_error>,
leaf::match<leaf::e_errno, ENOENT>,
leaf::e_file_name const & fn)
If it were possible to rewrite this into something like (and the same for catch)
[](leaf::match<error_code, input_file_open_error>,
leaf::match<leaf::e_errno, ENOENT>,
const leaf::XYZ& data)
where `XYZ` is the hypothetical type which can be queried for
`leaf::e_file_name` (or other additional info), then my greatest concern from
the "code reviewer" POV would be kind of addressed -- I can, for example, see
that the latter handler unconditionally handles an error code or errno.
________________________________
From: Stian Zeljko Vrba <[email protected]>
Sent: Saturday, May 30, 2020 12:16 PM
To: [email protected] <[email protected]>
Cc: Emil Dotchevski <[email protected]>
Subject: Re: [Boost-users] [review][LEAF] Review period ending soon - May 31
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 <[email protected]> on behalf of Emil
Dotchevski via Boost-users <[email protected]>
Sent: Friday, May 29, 2020 8:38 PM
To: [email protected] <[email protected]>
Cc: Emil Dotchevski <[email protected]>
Subject: Re: [Boost-users] [review][LEAF] Review period ending soon - May 31
On Thu, May 28, 2020 at 11:55 PM Stian Zeljko Vrba via Boost-users
<[email protected]<mailto:[email protected]>> 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)`?
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<my_error_code, err1, err2> )
{
// Handle err1 or err2 error
},
[]( leaf::match<my_error_code, err5> )
{
// Handle err5 errors
}
> 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_<file_read_error>, e_file_name const * fn )
{
if( fn )
// use e_file_name
else
// e_file_name not available
}
You could write a handler that takes all pointer arguments and then it would
match all errors and you can do your own logic.
_______________________________________________
Boost-users mailing list
[email protected]
https://lists.boost.org/mailman/listinfo.cgi/boost-users
