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

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 
regarding that.

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?
Didn't try
>    - 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?
Not really.