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

From: Stian Zeljko Vrba (vrba_at_[hidden])
Date: 2020-05-30 11:46:00

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 <vrba_at_[hidden]>
Sent: Saturday, May 30, 2020 12:16 PM
To: boost-users_at_[hidden] <boost-users_at_[hidden]>
Cc: Emil Dotchevski <emildotchevski_at_[hidden]>
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 <boost-users-bounces_at_[hidden]> on behalf of Emil Dotchevski via Boost-users <boost-users_at_[hidden]>
Sent: Friday, May 29, 2020 8:38 PM
To: boost-users_at_[hidden] <boost-users_at_[hidden]>
Cc: Emil Dotchevski <emildotchevski_at_[hidden]>
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 <boost-users_at_[hidden]<mailto:boost-users_at_[hidden]>> 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.