Re: [review][LEAF] Review of LEAF starts today : May 22 - May 31

From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2020-06-06 20:53:15

On Sat, Jun 6, 2020 at 4:21 AM Joaquin M López Muñoz via Boost <
boost_at_[hidden]> wrote:
> * If I understood it right, leaf::context, leaf::new_error and handlers
> can only take
> E-type arguments. Why is this so?

No longer the case; is_e_type is deleted on develop (the review branch
remains unchanged to avoid confusion).

> * I feel like is_result_type should be converted into a traits class
> (like allocator_traits)
> with value() and error() static functions so as to make it easier to
> adapt external
> error types into the framework.

Possibly. I prefer to not add complexity that is not strictly needed.

> * Using LEAF requires that called functions be wrapped/refactored so as
> to return a
> leaf::result<T> value.

Not a requirement. In fact one of the main design goals of LEAF is to
support transporting error objects of arbitrary types through intermediate
uncooperative layers of functions.

> Given that users are already expected to use
> LEAF_AUTO
> to reduce boilerplate, maybe this macro can be augmented like this:
>
>    namespace leaf{
>    template <typename T>
>    [[nodiscard]] result<T> wrap_result(result<T>&& x){return
std::move(x);}
>    template <typename T>
>    [[nodiscard]] result<T> wrap_result(T&& x){return ...;} // figure out
> what values of T are an error

Possible, however I'm exploring what I think is a better option to enable
this functionality. It is possible to make LEAF able use non-result<T>
types directly, without having to wrap them.

> * Maybe LEAF_AUTO and LEAF_CHECK can be unified into a single, variadic
> macro. Just
> a naming observation here.
>
> * Maybe there should be a (BOOST_)LEAF_ASSIGN macro to cover this:
>
>    LEAF_AUTO(f, file_open(file_name));
>    ... // closes f
>    LEAF_ASSIGN(f,file_open(another_file_name));

Possible, but I'd rather not facilitate reusing of local variables.

> * Whis is there a need for having separate try_handle_all and
> try_handle_some? Isn't
> try_handle_some basically equivalent to try_handle_all with a
> [](leaf::error_info const & unmatched) handler?

Two reasons:

1) try_handle_all ensures at compile time that the user has provided a
"catch all" handler. Consider that without this requirement, under
maintenance someone may delete it and now the program has a subtle,
difficult to detect bug.

2) because try_handle_all knows all errors are handled, it can unwrap the
result type and return a value.

> * I may be wrong, but I feel like error-handling and exception are
> completely separate.
> Is it not possible to have try_handle_all handle both
> [](leaf::match<...>) and
> [](leaf::catch_<...>) handers? The framework could statically determine
> whether there's
> a catch handler and then insert its try{try_block()}catch{...} thing
> only in this case, so as to
> be exception-agnostic when needed.

That is exactly how LEAF works. Use catch_ in any of your handlers, and
try_handle_all/some will catch exceptions, otherwise they won't (the third
alternative, try_catch, always catches exceptions, and does not use a
result type).

> I'm focusing here on code/naming  guidelines in the context of Boost
> libraries:
>
> * LEAF macros (both public and internal) should be BOOST_-prefixed.

Of course.

> * There are macros with the same semantics as Boost-level macros:
> BOOST_NO_EXCEPTIONS,
> BOOST_NO_THREADS. The latter should be used insstead i this is to become
> a Boost library.

Yes, integration with Boost Config is TBD.

> * There are chunks of Boost source code embedded into the project
> (test/boost/core).
> Needless to say this should be removed.

Fixed on develop.

> * s/lEAF_NODISCARD/LEAF_NODISCARD in
>
https://github.com/zajo/leaf/blob/76edf8edc3a17be3ac4064f3cc18d7c6a4e86b4c/include/boost/leaf/config.hpp#L126

Ouch. Thanks, fixed on develop.

> *  LEAF_AUTO(v,r) defines a variable with name _r_##v. Identifiers
> beginning with "_r"
> are not strictly reserved by the C++ standard (they are reserved only at
> global scope
> and within std namespace), but I think it is bad practice to have them
> anyway.

The idea is to minimize the chance of a name clash, hoping that users know
to avoid such names

> etc. Although the docs do not stress it, I think it can also play a role
> when mixing
> error-returning and exception-throwing code.

The docs mention this. There is also leaf::exception_to_result, see this
example:
https://github.com/zajo/leaf/blob/master/examples/exception_to_result.cpp?ts=4

Thanks!