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

From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2020-06-02 09:50:21

>> 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've explored folding it all into try_handle_all, but the implementation
> gets really messy. Perhaps it can be done. Consider that in this case the
> loop that matches the lambdas based on what error objects are available at
> runtime now has to be recursive. Maybe it can be done elegantly, but I
> tried and gave up.
Naive approach: Make the aggregate handler a type with variadic template 
params for the handlers and overload try_handle_all based on that. One 
can then call the other to avoid code duplication
I'd also like to remind you of the first part of the proposal where 
`remote_handle_all` does not require the user to create a lambda which 
IMO considerably improves the UX.
Reason for bringing this up again is that once it is released it can't 
really be changed (or only at a higher cost) and IMO eliminating 
something already identified as "unintuitive" is worth delaying the 
release for a bit for exploring other approaches.
>> - "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)
> This is a somewhat-known problem with exceptions. If you have:
>
> struct A { };
> struct B1: A { };
> struct B2: A { };
> struct C: B1, B2 { }
Ok, so you are referring to diamond-style inheritance hierarchies which 
are for the reason you mentioned often avoided. I feel like this doesn't 
come up to often for exception types as they are usually flat 
hierarchies (speaking from my experience, YMMV) Hence I'd recommend not 
bringing this up in the documentation as I find it more distracting than 
helpful.
>> - "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)
> Normally you give LEAF an object of type T and it stores it away for you.
> If you later give it another value of type T, it'll just overwrite the old
> one. With accumulate, you get a mutable reference to the stored value (and
> it requires T to have a default constructor).
Thank you for the explanation. Can you show an example in the docu for a 
use case? I'm having a hard time imagining one as usually errors are 
more like "I failed to open file x" and not "I failed to open file x, y & z"
Having a more complete example use case where accumulate is actually 
used to store (and retrieve) multiple values could be helpful for the user
>> - "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,"
> The logic is the same as for preload, which takes your value, and will
> later communicate it if an error has occurred, while defer takes your
> lambda and will later call it to get a value to communicate, if an error
> has occurred.
May I suggest to make this more clear in the documentation? Like "On 
error, the function is called in the destructor, else it is not called" 
int that sentence.
>> 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.
> Yes, though leaf::result<T>, in case of an error, only transports a single
> int, the error_id, which allows for efficient implementation.
>
> That said, you can use pretty much any result type with LEAF, it does not
> have to be leaf::result<T>.

Sorry I might have been not fully clear, allow me to be more specific: I 
wasn't referring to lead::result<T> but to the concept of result<T> as 
found in this and other exception handling libraries. With an API like 
`result<Foo> do() noexcept` instead of `Foo do()` you won't get (N)RVO 
when you return a `Foo` (in the happy case) as the standard only allows 
it when the types are exactly the same.
So my question was what impact the missing RVO has on performance in the 
happy path compared to the "standard" approach of using RVO-enabled 
functions and (potential) exceptions.