From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2020-05-29 08:26:16
Disclaimer: This is from someone not using Boost.Exception, Expected or
any other result<T> library and judging from the documentation. So this
might serve in evaluating how well the lib/documentation fares to
someone thinking to adopt this.
> - What is your evaluation of the design?
There is a mixture of lambdas and macros which I find a bit off-putting
on first sight. However it seems they solve the problem well in the
constraints of the language.
I find the default for `is_e_type` being true for any X having X::value
a bit broad. This basically applies to all strong types so it feels like
an "E-Type" could be anything. So I don't understand why the distinction
into E-types and non-E-types is made.
Maybe improve on `remote_handle_all` in 2 ways: Create the lambda taking
`leaf::error_info const&` in that function to remove the boilerplate at
the user call site. Given that rename it to e.g. `create_remote_handler`
or so. I find the current interface confusing and the documentation
calls it "unintuitive", so I guess this should be improved on before
release. Given that: It seems `remote_try_handle_all` can be folded into
the regular `try_handle_all`. So `try_handle_all` takes a list of
handlers where one could be the aggregated handler defined by
`create_remote_handler` (then maybe `create_aggregate_handler`?)
I'm interested in knowing how `try_handle_all` implements "if the user
fails to supply at least one handler that will match any error, the
result is a compile error". If arbitrary functions can be called
throwing any error type, how could that work? Is it again required to
use a catch-all clause?
> - What is your evaluation of the documentation?
Good overall, but the details are to detailed for an introduction and to
coarse for understanding it.
Examples by header:
- "Using Exception Handling": virtual inheritance is mentioned but the
reasoning is not clear to me. I have never used or seen it used in that
context and always strive to avoid it due to the issues with handling
such an object (starting at writing ctors)
- "Accumulation": shows how to call leaf::accumulate but I don't see
what it use is. "Many different files" are mentioned but the operation
only takes a single file. And based on everything before a single error
stops execution (like exceptions)
- "E-types": I didn't got what an E-type is exactly. It starts with
"values" you can add to "errors" or "exceptions" but seemingly
exceptions itself are "E-types"? See also above. I also didn't see the
enums being mentioned here that are used in other places. This would be
a great addition
- "Loading": It starts how values are put into a `context`. I feel that
a detailed introduction how a context works and is used/accessed prior
to this would be useful. My point is: The `leaf::load` call is somewhere
down the chain not knowing anything about the `try_handle_*` call and
hence the context. How does it know about it? Or what to put there? So
maybe start a first paragraph solely on the context and its access
- "error_id": It is unclear to me what it's use is. Especially as
something unique in the program in the context of threads means some
kind of synchronization which will be paid for even if unused.
- "defer": I'd like to know when the lambda is not called. The
documentation says it is called in the dtor, but later: "function passed
to defer, if invoked,"
- "Working with Disparate Error Types": To me it is unclear what the
message of this part is. Maybe it can be shortened.
- "Lua": Code example is missing the namespace for `augment_id` and
there is likely a typo "aughent"
Wording at some places might be improved.
- "With LEAF, users can efficiently associate with errors or with
exceptions any number of values that pertain to a failure", for me this
is hard to understand.
- "Letâs say we want to build a record of file locations a given error
passes through on its way to be handled", is something missing here?
As it is a C++11 library I'd suggest to not use plain enums in the
documentation. E.g. `enum do_work_error_code` in the lua example with
such generic names. This might lead beginners into C&P the code.
> - What is your evaluation of the potential usefulness of the library?
It seems to solve the same problem as other error handling libraries but
in a new way.
Performance-wise it seems to be comparable. I'd suggest to evaluate the
cases where it lacks behind and whether those can be improved
Giving the intention to unify error handling (or try to make it more
uniform) I'm wondering if this could be standardized. Given that Boost
is often a "playground for future C++ standards" I feel this should be
evaluated whether this is possible at all. Of course I'd love to see
language support avoiding lambdas and macros, but I'm not optimistic
I'd also like to see a comparison with exceptions especially in cases
where the usage of result<T> inhibits (N)RVO (even more for C++17 and
up). This is my main critic point about result<T> type of error handling
as that variant type creates less efficient code for the happy path.
All in all I think it is useful to experiment with a new approach and as
such fits Boost
> - Did you try to use the library? With which compiler(s)? Did you
> have any problems?
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
In-depth reading of documentation only.
> - Are you knowledgeable about the problem domain?
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk