On Sat, Jun 6, 2020 at 4:21 AM Joaquin M López Muñoz via Boost < boost@lists.boost.org> 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/i... 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... Thanks!