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 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
