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

Subject: Re: [boost] [Fit] formal review starts today
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2016-03-05 03:36:02

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

Vicente