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 18:07:38

Le 05/03/2016 16:48, Paul Fultz II a écrit :
>
>
> 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:
>
>
> std::ref just stores a pointer to the object, whereas fit::mutable_ 
> stores a copy of the object.
I see.
>
>     >> 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 <http://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.
Please, could you add an issue, if not already done?

Vicente