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:58:48

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

That and it works with constexpr. Compressed pair is an implementation 
detail and is not part of the interface.
 

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

I do mention that they support `constexpr`.
 

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

Yes, I need to document this rationale.
 

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

Its not an interface change. Its just a workaround for gcc.
 

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

This is not really an interface change. The change would be backwards 
compatible.