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

Subject: Re: [boost] [Fit] formal review starts today
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-05 10:50:42

On Saturday, March 5, 2016 at 3:02:18 AM UTC-6, Vicente J. Botet Escriba 
wrote:
>
> 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.
>

I do confirm the issues pointed it out, I just didn't feel the need to 
repeat myself after everyline. Sorry.
 

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

Alright, I will do that.
 

> Vicente
>
> _______________________________________________
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost
>
>