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

On Saturday, March 5, 2016 at 2:36:28 AM UTC-6, Vicente J. Botet Escriba 
wrote:
>
> Le 05/03/2016 01:05, Paul Fultz II a écrit : 
> > 
> > On Friday, March 4, 2016 at 5:00:32 PM UTC-6, Steven Watanabe wrote: 
> >> AMDG 
> >> 
> >> On 03/03/2016 07:16 PM, paul Fultz wrote: 
> >>>> <snip> 
> >>>> 
> >>>> - 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`. 
> >>> 
> >>>> It's especially 
> >>>>    weird when a function (e.g. partial) requires 
> >>>>    both ConstCallable, and MoveConstructible. 
> >>> Almost all of the function adaptor require that. Why is that weird? 
> >>> 
> >>    The fact that you require MoveConstructible 
> >> implies that you're carrying around a copy. 
> >> 
> > Yes that's correct. 
> >   
> > 
> >> All the standard library algorithms take function 
> >> objects by value, and do not constify them. 
> >> 
> > Yep, and there's no guarantee how many times it gets copied in an 
> algorithm. 
> > Which means using mutable objects could return different result 
> depending 
> > how 
> > the algorithm decided to copy the function. This becomes even more 
> apparent 
> > when people start using function adaptors as well. 
> I would like to see an example where the problem is evident. 
> > Because of these issues I decided to make mutability explicit. So the 
> user 
> > will need to write `std::ref` or `boost::fit::mutable_` to use a mutable 
> > function. 
> >   
> It is a good practice to document the motivation of your design and show 
> how the problem disappear. 
> BTW, how fit::mutable_ is different from std::ref? 
>

std::ref just stores a pointer to the object, whereas fit::mutable_ stores 
a copy of the object.
 

> >> std::not1/2 (and also the obsolete std::bind1st/2nd) 
> >> do require const like you do, but std::bind does not, 
> >> and std::function even has a const operator(), 
> >> without requiring the same from its argument. 
> >> 
> > Yep, and std::bind suffers the same issue. 
> >   
> > 
> >>>> <snip> 
> >>>> You should not be requiring 
> >>>>    const at all, for anything.  See [func.bind.bind] 
> >>>>    for an interface that handles const correctly. 
> >>> I probably should add Q/A for this. I make mutability explicit because 
> >> for 
> >>> most cases `std::ref` should be used when using a mutable function 
> >> object. If 
> >>> its really necessary(for example working with a third-party library) 
> >> then 
> >>> `mutable_` adaptor can be used. 
> >>> 
> >>    You don't get to make up your own rules 
> >> when you're dealing with long established 
> >> concepts like function objects. 
> >> 
> > Whats so problematic with explicit mutability beyond that's not the way 
> > std::bind works? 
> >   
> There is no problem adding explicit mutability when it solves an problem. 
> >>>> <snip> 
> >>>> 
> >>>> apply_eval.hpp: 
> >>>> 
> >>>> 18: "Each [`eval`](eval.md) call is always ordered 
> >>>>      from left-to-right." 
> >>>>      It isn't clear to me why this guarantee is important. 
> >>>>      Not to mention that the test case apply_eval.cpp 
> >>>>      doesn't check it. 
> >>> That is the whole purpose of apply_eval, so the user can order the 
> >> parameters 
> >>> that are applied to a function. 
> >>> 
> >>    Yes, but /why/?  The name apply_eval 
> >> doesn't convey any notion of evaluation 
> >> order to me.  It only matters for functions 
> >> with side effects, and I wouldn't normally 
> >> be using apply_eval on such functions in 
> >> the first place.  The examples that you 
> >> provide don't depend on this, either. 
> >> 
> > I probably should find a better example. 
> >   
> We will need this better example during the review to be able to say if 
> it is useful. 
> > 
> >>>> <snip> 
> >>>> 
> >>>> 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) 
> >>>>      You need parens around the comma operator. 
> >>>>      Also not tested. 
> >>> For what? 
> >>> 
> >> In context: 
> >> int x; 
> >> template<class F, class... Ts> 
> >> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, 
> >> BOOST_FIT_FORWARD(Ts)(xs)...), 0) 
> >> {} 
> >> 
> >> This constructor argument of x is essentially like: 
> >>    int x(1, 3); 
> >> which is ill-formed. 
> >> 
> >> I'm presuming that you intended to use the 
> >> comma operator, like so: 
> >>    x((apply(...), 0)) 
> >> 
> >>    You didn't detect the problem because 
> >> this template is never instantiated by 
> >> your tests. 
> >> 
> >   
> > The tests don't run this because of `BOOST_FIT_NO_CONSTEXPR_VOID` is 
> defined 
> > to 1, for all platforms. However, there are tests for this. 
> >   
> Please, could you point to them? 
> Could you add an issue to tract the configuration of a test with 
> BOOST_FIT_NO_CONSTEXPR_VOID 0? 
>
 
There is a line that needs to be removed in the source code, which defines 
it
to 1, this disables the configuration of the macro based on the platform. I
didn't realized that this line was in the source code. Essentially, what
happens is that it falls back to using an alternative for `void` even when 
its
supported on newer platforms. There is a test for `apply_eval` at line 15,
that checks it being called with a function returning `void`.

So, essentially, the code pointed out by steven is never used by the 
library.
However, once I remove the define, the tests will fail, and I will need to
correct the problem.
 

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