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

Subject: Re: [boost] [Fit] formal review starts today
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2016-03-04 18:00:05

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.
All the standard library algorithms take function
objects by value, and do not constify them.
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.

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

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

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

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

>>
>> 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);

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

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