El 22/05/2020 a las 11:31, Michael Caisse via Boost escribió:
The Boost formal review of Emil Dotchevski's LEAF (Lightweight Error Augmentation Framework) library will take place from May 22 through May 31st.
Hi, this is my late, unexhaustive review of LEAF. I didn't have the time to make this more structured, so each section below is more of a list of issues and ideas that I came up with while reading the docs and inspecting the code. *Design* * If I understood it right, leaf::context, leaf::new_error and handlers can only take E-type arguments. Why is this so? I'm sure there's a reason for that, but at first blush seems like anything (which is moveable etc.) could be potentially accepted and automatically handled by the framework, which would eliminate the need for some user-side boilerplate. * 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. Also, instead of is_result_type<T> specializing to std::false_type by default, some metaprogramming gymnastics can be played to try to deduce if T provides a compatible interface. * Using LEAF requires that called functions be wrapped/refactored so as to return a leaf::result<T> value. 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 #define LEAF_AUTO(v,r)\ auto && _r_##v = leaf::wrap_result(r);\ if( !_r_##v )\ return _r_##v.error();\ auto & v = _r_##v.value() } where non leaf::result-returning functions are automatically wrapped. The tricky part is the second overload of wrap_result, which could have some default semantics for what constitutes an erroneous value (for instance, !x evaluating to true) and then be overloadable by the user: defining the semantics for wrap_value<some_type> once is less boilerplate than wrapping all some_type-returning functions. * 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)); * 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? * 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. * I didn't study the thread-transporting area of the framework. * To summarize on the design aspects, I think that: * LEAF can do more to alleviate the required boilerplate when adapting legacy code to this new framework. * Error return values and exception should ideally be handled in a more unified way. There's certainly code that uses libraries with both error-reporting styles, and LEAF should give service to this. *Implementation* I'm focusing here on code/naming guidelines in the context of Boost libraries: * LEAF macros (both public and internal) should be BOOST_-prefixed. * 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. * There's a good deal of __clang__, __GNUC__, _WIN32, etc. checking that should be replaced with the corresponding Boost.Config macros (when possible). * There are chunks of Boost source code embedded into the project (test/boost/core). Needless to say this should be removed. * s/lEAF_NODISCARD/LEAF_NODISCARD in https://github.com/zajo/leaf/blob/76edf8edc3a17be3ac4064f3cc18d7c6a4e86b4c/i... * 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. * In the same vein, I was shocked by code like "auto _ = {...};". * To summarize, the implementation looks clean and dandy, but it shows this is not a Boost lib yet and more work should be done in that direction. *Documentation* * A good deal of work has been put into the docs. They look good and manage to explain hard concepts in a very approachable fashion. *Usefulness of the library* I think this is a potentially very useful library with all the ongoing conversation around exception haters (gamedevs, embedded), Sutter's deterministic exceptions proposal, etc. Although the docs do not stress it, I think it can also play a role when mixing error-returning and exception-throwing code. *Other review questions** * * Did you try to use the library? With which compiler(s)? Did you have any problems? I didn't try it * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Three hours reading the docs and the code * Are you knowledgeable about the problem domain? Certainly not an expert. *Vote* I weakly vote for REJECTION at its current state. I like LEAF very much and there's a lot offered here, but I think there are some design considerations that are best handled before acceptance, when the author has more leeway for experimentation, so as to have a resoundingly successful second coming. Joaquín M López Muñoz