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

From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2020-05-29 18:38:34

On Thu, May 28, 2020 at 11:55 PM Stian Zeljko Vrba via Boost-users <
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.