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 19:05:03

On Saturday, March 5, 2016 at 5:07:58 PM UTC-6, Vicente J. Botet Escriba 
wrote:
>
> 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?

That is part of this issue here: https://github.com/pfultz2/Fit/issues/113
 

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