Re: [boost] [Fit]Â formal review starts today

Subject: Re: [boost] [Fit] formal review starts today
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2016-03-05 04:01:57

Le 04/03/2016 21:27, Paul Fultz II a écrit :

Paul, snip the parts you don't replay to.
And replay to the parts you acknowledge.
If it you that create an issue, is a sign that you confirm already that 
the is an issue that must be addressed.
>
>
> On Friday, March 4, 2016 at 12:56:05 PM UTC-6, Vicente J. Botet 
> Escriba wrote:
>
>     Le 04/03/2016 03:16, paul Fultz a écrit :
>     >> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe
>     <watan..._at_[hidden] <javascript:>> wrote:
>     >>> AMDG
>     Thanks Steven for all these useful remarks.
>     >>> - How much effort did you put into your evaluation of the review?
>     >>>
>     >> ~5 hours.  I only finished half a dozen
>     >> files, but I've seen enough problems that
>     >> I feel justified in stopping now.
>     >>
>     >> Critical points:
>     >>
>     >> - Different functions in the library do not
>     >>    have a consistent interface for similar
>     >>    variations:
>     >>    - always/always_ref vs.
>     capture/capture_forward/capture_decay vs
>     >> partial w/ std::ref.
>     >>
>     >> - The library reinvents (poorly) its own versions
>     >>    of several components:
>     >>    compressed_pair <-> boost::compressed_pair,
>     > As I recall boost::compressed_pair is never empty. In addition,
>     it doesn't
>     > support constexpr. Plus, I structured it in a way to avoid bugs
>     with gcc when
>     > doing EBO with constexpr.
>     Could you provide a patch for boost::compressed_pair that is backward
>     compatible and provides whatever you need?
>
>
> Nope. Compressed pair is designed such that &x.first() != &x.second() 
> is always the case. As some users may make assumptions about that.
Is this the single difference?
>
>     >>    placeholders <-> lambda, bind, phoenix
>     > Doesn't work with constexpr, and/or is not SFINAE-friendly nor
>     does it handle
>     > perfect forwarding for more than 2 arguments.
>     The same here.
>
>
> Making bind SFINAE-friendly breaks backwards compatibility. However, 
> the lazy adapter passes the same tests for Boost.Bind, plus with 
> constexpr.
This should be documented as motivation of the addition of new placeholders.
>
>     >
>     >> - Some library functions require function objects
>     >>    to have a const operator(), without any rhyme
>     >>    or reason.  e.g. apply requires Callable, but
>     >>    apply_eval requires ConstCallable.
>     > This is partly because of the fold done for older compiler. I
>     could easily
>     > lift this restriction. Almost all of the adaptors use
>     `ConstCallable`.
>     IIUC Steven, he is requesting to use Callable everywhere. Steven
>     could
>     you confirm?
>
>
> The library could switch to using `Callable`, if everyone feels that 
> is better. However, I honestly prefer to make mutable explicit. I 
> would like to hear feedback from other reviewers as well.
We need to see the motivation for explicit mutable function objects.
How your library work with standard function objects and other that user 
have written that conform to the standard requirements?
>
>     >
>     >> 107: I have no idea why you made alias_value variadic.
>     >>      The extra arguments are unused,  their presence is
>     >>      undocumented, and all calls from inside the library
>     >>      (pack.hpp, combine.hpp, and detail/compressed_pair.hpp)
>     >>      that use them, could just as well be written without.
>     > This is to make `alias_value` depend on a template parameter
>     because compiler
>     > eagerly instantiate template parameters.
>     Please could a clear explanation be added in the documentation?
>
>
> I don't know if it should be added to the documentation as its a 
> implementation detail. Perhaps an extra comment could help.
Anything that justify an interface change should be described in the 
documentation.
>
>     >
>     >> arg.hpp:
>     >>
>     >> 29: ///    template<class IntegralConstant>
>     >>      ///    constexpr auto arg(IntegralConstant);
>     >>      ///
>     >>      ///    template<std::size_t N, class... Ts>
>     >>      ///    constexpr auto arg_c(Ts&&...);
>     >>      arg and argc should do the same thing.  It looks
>     >>      like arg creates a function object, while arg_c
>     >>      evaluates it directly.
>     > Yes, this because you write `arg(int_<5>)(...)` and
>     `arg_c<5>(...)`. In the
>     > future, I may make this a template variable(so `arg_c<5>` will
>     be a function
>     > object).
>     What prevents you to doing it now?
>
>
> Time constraints mainly, but this applies to both arg_c and if_c.
If this would  change the interface it is better to do it before the 
library is accepted.
If not could this be added to a future work section?
>
>
>     Thanks again Steven.
>
>     Even if you are not for the library in its current state your
>     comments
>     will help a lot to improve it, so please, could you reconsider to
>     give
>     us more feedback on other parts of the library?
>
>
> Yes this feedback was very useful, and would appreciate more feedback 
> as well. I will open issues for the issues raised currently. More 
> issues can be raised directly on the github page.
>
I agree, but I would like to see them with a review label and I believe 
that only you can assign the labels.
This will be very helpful to me.

Vicente