Subject: Re: [boost] [Fit]Â formal review starts today
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2016-03-04 18:00:05
On 03/03/2016 07:16 PM, paul Fultz wrote:
>> - 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.
>> 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.
>> - [ 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:
test-suite is at the bottom of:
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.
>> 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.
>> 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0)
>> You need parens around the comma operator.
>> Also not tested.
> For what?
template<class F, class... Ts>
constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f,
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:
You didn't detect the problem because
this template is never instantiated by
>> 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
Making arg_c a variable template would answer my concerns.
>> 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.
>> 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
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.
>> 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk