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

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.

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.
 

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

>
> >> <snip> 
> >> 
> >> Details: 
> >> 
> >> <snip> 
> >> - [ test_all r ] 
> >>   "r" is completely meaningless here. 
> > 
> > That is the way it was from where I copied it. 
> > 
>
>   Then your source is probably also wrong. 
> All it does is to pass the string "r" as 
> an argument to test_all, but since test_all 
> doesn't use any arguments, it has no effect. 
>
> >> - Actually, everything from rule test-all 
> >>   down can be reduced to: 
> >>   for local fileb in [ glob *.cpp ] 
> >>   { 
> >>     run $(fileb) ; 
> >>   } 
> >>   You don't need to create a special target 
> >>   to group the tests, unless you want more 
> >>   granularity than "run everything" or "run 
> >>   specific tests by name." 
> > 
> > Interesting. I really could find no reference on this information. 
> > 
>
> run etc. are documented here: 
> http://www.boost.org/build/doc/html/bbv2/builtins/testing.html 
> test-suite is at the bottom of: 
> http://www.boost.org/build/doc/html/bbv2/reference/rules.html 
>
>   Once upon a time, I believe that test-suite 
> was actually necessary.  It has survived even 
> to the present day, because most people create 
> Jamfiles by copy-pasting existing Jamfiles. 
>
> >> 
> >> <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.
 

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

>
> >> 
> >> 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). 
> > 
>
> Making arg_c a variable template would answer my concerns. 
>
> >> <snip> 
> >> 
> >> by.hpp: 
> >> 
> >> 
> >> 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)...; 
> > 
>
>   Sure they're similar, but the name "by" 
> makes no sense for the latter. 
>
> >> 
> >> 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. 
> > 
>
>   My problem is that apply_eval already 
> does all the necessary #ifdefing. 
> Scattering #ifdefs around just makes 
> the code unmaintainable. 
>
> >> 
> >> 135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN 
> >> 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow 
> >>     You should really have a single BOOST_FIT_VOID_RETURN 
> >>     and use it consistently everywhere. 
> > 
> > The BOOST_FIT_BY_VOID_RETURN is only used in one place so it could be 
> replaced 
> > with preprocessor ifs. 
> > 
>
>   My point is that having multiple void_ types 
> means that you have to keep track of which void_ 
> you need depending on which other functions 
> you use as implementation details. 
>

I can move to changing this to one void type.
 

>
> >> 
> >> capture.hpp: 
> >> 
> >> 32: // Capture lvalues by reference and rvalues by value. 
> >>     The default should be to capture everything by value. 
> >>     Capturing by reference is potentially dangerous, and 
> >>     switching based on whether the argument is an lvalue 
> >>     is even more so. 
> > 
> > I don't see how it makes it more dangerous. Everything in scope is 
> captured by 
> > reference and temporaries have there lifetimes extened so they will 
> remain in 
> > scope as well. This works great when forwarding is added on. 
> > 
>
>   In general a function that accepts both 
> lvalues and rvalues should have the same 
> behavior for both, modulo optimizations. 
> Anything that doesn't is just asking for 
> trouble. 
>
>   In particular, I claim that if the function 
> g returns a temporary, then the following 
> transformation should either be valid 
> and should leave the behavior unchanged, 
> or it should fail to compile: 
> f(g()); => auto x = g(); f(x); 
>

I don't see how that prevents that.
 

>
> >> 
> >> 79: return always_ref(*this)(xs...); 
> >>     Why not return *this? 
> >>     The always_ref dance serves no purpose, 
> >>     other than making the code hard to follow. 
> > 
> > This is necessary because of the eagerness of constexpr. 
> > 
>
>   I changed it and your tests still passed. 
> I suspect that the problem that required this 
> workaround was when you used BOOST_FIT_RETURNS 
> or otherwise used this construct in the function 
> prototype.  There shouldn't be any problems 
> when it only appears in the function body, as 
> it does here. 
>

There are probably some places where this can removed. I mostly like did it
that way for consistency.
 

>
> >> 
> >> placeholders.hpp: 
> >> 
> >> We really don't need another lambda library, let alone 
> >> a half-baked one. 
> > 
> > Its not designed to be a full lambda library. Just enough to handle some 
> > constexpr cases. 
> > 
>
> Exactly my point. 
>
> In Christ, 
> Steven Watanabe 
>
>
> _______________________________________________ 
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost 
>