Boost logo

Boost :

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

>> <snip>
>> apply_eval.hpp:
>> 18: "Each [`eval`]( 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.

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

  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

Boost list run by bdawes at, gregod at, cpdaniel at, john at