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://listarchives.boost.org/mailman/listinfo.cgi/boost
>
>