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-04 15:27:08

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.
 

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

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

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

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

> > 
> >> 86: make_args_at(...)(..., make_perfect_ref(...)...) 
> >>      ADL... 
> >> 
> >> 95: get_args<N>(...) 
> >>      ADL 
> Associated those to the ADL issue. 
> >> by.hpp: 
> >> 
> >> 8: BOOST_FIT_GUARD_FUNCTION_ON_H 
> >>    The #include guard doesn't match the file name 
> > It originally was called `on`, thats why. 
> Please, could you create an issue? 
> > 
> >> 22: "Also, if just a projection is given, then the projection will 
> >>      be called for each of its arguments." 
> >>      I don't see any good reason for this to be an 
> >>      overload of by.  It's essentially a for_each. 
> > It seems like a natural extension: 
> > 
> > by(p, f)(xs...) == f(p(xs)...) 
> > by(p)(xs...) == p(xs)...; 
> > 
> >> 25: Note: All projections are always evaluated in order from 
> left-to-right. 
> >>      By only takes one projection. 
> > Yea, that should read all the applications of the projection. 
> Please, could you create an issue? 
> >> This is a lot of code to implement what is essentially a one-liner: 
> >>    apply_eval(f, capture_forward(x)(p)...) 
> > It does that for older compilers, but on compilers that support it, it 
> uses 
> > initializers lists. 
> What are the old compilers? I don't know if it will be worth to have a 
> branch for compilers conforming to C++14 and one for others. 
>

Well, MSVC and gcc 4.8 is included in the older compilers, but I can make 
it work with Callable.
 

>
> >> concepts.md: 
> >> 
> >> Please reference the C++ standard when defining INVOKE. 
> >> (Unless of course your INVOKE is different, which it 
> >> had better not be) 
> > Yes it is the same, I'll try to find the reference to the standard. 
> > 
> > 
> Maybe you can reference 
> http://en.cppreference.com/w/cpp/utility/functional/invoke 
>
> There is already an issue for this point. 
>
> 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.

Thanks,
Paul