Boost :

Date view	Thread view	Subject view	Author view

Subject: [boost] Fit review - viboes
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-09-09 11:27:24

Next message: Jason Roehm: "Re: [boost] [build] using nvcc as toolset target?"
Previous message: Richard Hodges: "Re: [boost] Review Request: impl_ptr (pimpl)"
Next in thread: P F: "Re: [boost] Fit review - viboes"
Reply: P F: "Re: [boost] Fit review - viboes"

Hi,

here it is my review.

I believe that Boost.Fit should be*accepted*.

*Design*

The design is sound and clean.

I have some concerns respect the the ConstFunctionObjects that I believe should merit an explanation on a rationale section. Why the library support only Const function objects?

Wondering if some parts should be moved to Boost.TypeTraits or reused from there (is_callable).

I'm missing the std::invoke function.

The_adaptor<> are not documented. The utility of those classes is not
documented. If the user can use those classes the documentaion must
document them. I guess they are useful to build other adaptors. It is
not clear whether the defined functions would use SFINAE or just fail
when used with types that don't respect the requirements. While I'm
generally for SFINAE, I understand that it will not be the case in order
to improve the compile time. It is not clear if the functions can be
final function objects or not.
*Implementation*

Not seen in depth.

It should use as much as possible Boost.Config and/or Boost.Predef

Possible files contains details that could be shared in Boost or used from other Boost libraries

It uses a lot of macros that make complex the understanding of what is behind. On the same file https://github.com/pfultz2/Fit/blob/boost/include/boost/fit/compose.hpp and only on 169 lines
BOOST_FIT_INHERIT_CONSTRUCTOR, BOOST_FIT_RETURNS_CLASS,
BOOST_FIT_SFINAE_RESULT, BOOST_FIT_MANGLE_CAST,
BOOST_FIT_DECLARE_STATIC_VAR, BOOST_FIT_NOEXCEPT_CONSTRUCTIBLE,
BOOST_FIT_ENABLE_IF_CONSTRUCTIBLE, BOOST_FIT_JOIN *Documentation*

Quite good butthe description of the semantic of the provided functions would need
some extra wording. Maybe a section that states that the semantic
expression is part of the constraints on the types. It takes too much
pages to see what the library proposes.
For some functions we would need more information, examples. See below the detailed

*Potential usefulness of the library*

Very useful.

*Did you try to use the library? With which compiler(s)? Did you have any
problems?***
Not tried yet.

*How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?***
In-depth study of the documentation (about 5 h for this revision).

*Are you knowledgeable about the problem domain?*

In general, yes.

*Were the concerns from the March 2016 review of Fit addressed?***
I believe ;-)

Best,
Vicente

*Documentation:*

In general:

Â Â Â * Navigation only from the top of the page

Â Â Â * Hyper links missing.

Â Â Â * There is no link to real code from the examples.

In particular:

*http://pfultz2.github.io/Fit/doc/html/doc/src/gettingstarted.html*

An explanation of the BOOST_FIT_LIFT limitations would be more than welcome.

Typo: last right parenthesis in

// Pipable sum
BOOST_FIT_STATIC_LAMBDA_FUNCTION(sum) = pipable([](auto x, auto y)
{
return x + y;
});

It is not clear if the following is correct in the body of a function

autosum = pipable([](auto x, auto y)
{
return x + y;
};

*http://pfultz2.github.io/Fit/doc/html/doc/src/example_print.html*

I would suggest to use an heterogeneous tuple in the for_each_tupleexample.

In addition use some kind of brackets to print the sequences and the
tuples would help to identify what is been printed.

*http://pfultz2.github.io/Fit/doc/html/doc/src/example_overloading.html*

Typo:

Before
// Check that T has member function for operator* and ope After // Check
that T has member function for operator* and opeerator->

I'm not sure the example is_derreferenceable is fair.

*http://pfultz2.github.io/Fit/doc/html/doc/src/example_polymorphic_constructors.html*

Maybe it is worth adding references to the proposals (P0318R0 and P0338R2)

*http://pfultz2.github.io/Fit/doc/html/doc/src/more_examples.html*

Extension Methods

It is not clear what numbers, filter and transform are in the example.

I believe that the range proposal woul include the pipe operator.

My question is how both pipe operator overloads interact.

http://pfultz2.github.io/Fit/doc/html/doc/src/point_free.html

typo

Before

|b(f)(x, y)|

|after|

||by(f)(x, y)||

*http://pfultz2.github.io/Fit/doc/html/doc/src/definitions.html*

It woul dbe great to have an example associated to each definition

Static Function Adaptor

I would say "It has an additional requirement that the
*class****function* is |DefaultConstructible|:"

Decorator
I would say "The *resulting* Function Adaptor
<http://pfultz2.github.io/Fit/doc/html/doc/src/definitions.html#function-adaptor>
may be an unspecified or private type."

Typo?
Some parts of the documentation provides

*http://pfultz2.github.io/Fit/doc/html/doc/src/concepts.html*
ConstFunctionObject

I would like to note that this concept is not representable using the
Concept TS as it is quantifying universally on the Ts (args) parameters.

The same applies to UnaryFunctionObject, BinaryFunctionObject

Wondering if the documentation should use the term Concepts then.

The use of expresion "Is an object with ..." is confusing. What are we
qualifying the type or the instance?

Maybe it is worth defining FunctionObject.

EvaluatableFunctionObject

It is not clear why this kind of FunctionObjects is useful

Callable

I believe we have moved to call this Invocable in the standard. But
again, I believe that you want to quantify universally the arguments,
isn't it?

Does your definition of INVOKE differs from the one on the standard? If
yes, the differences should be mentioned. If not, the definition on the
standard should be referenced.

You use type T twice with different meaning

"The type |T| satisfies |Callable| if"

"if |f| is a pointer to member function of class |T|:"

I believe that here the T are not the same.

Are final function objects Callable, ConstCallable?

ConstCallable
The brief description is the same as the one for Callable
Is the definition of the second INVOKE different from the first one?

UnaryCallable
The name doesn't convey that it is *Const*Callable

Metafunction and MetafunctionClass
it is not clear what those are, there is no brief description. I guess
that this applies to f.
What do you mean by a type or a template?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/by.html*

It is not clear to me what

by(p)(xs...) == p(xs)... mean

An example of `fit::by` without a function argument will be welcome.

It is not clear how by_adaptor<Projection, F> and by_adaptor<Projection>
can be used by the user? Shouldn't those be hidden?

This is a genral remak, why F must be ConstCallable?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/compose.html
*The requirements don't state clearly the constraints on the input
parameters of f and the result of g, except by the semantic expression.
This is a general remark. I suspect that you mean the semantic
expression must be well formed. But, is this SFINAE friendly or is this
a requirement? Can both function be final function objects? ****

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/conditional.html*
It is not clear what king of parameters this function could have (in
particular in comparison with P0051). Does it accept function/data
members pointers, final classes, references_wrappers, ...?

Typo "Proposal for C++ Proposal for"
P0051 original proposal uses overload_linearly/overload (as Boost.Hana)
instead of conditional/match. Wondering if those names aren't more
clear. Why do we need to change the Boost.Hana names? There is also
std::conditional :(

Can more than 2 parameters be final function objects?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/flip.html**
*
IIUC, F must not be BinaryCallable, but Callable with at least two
arguments.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/implicit.html*

Is implicit a StaticFunctionAdaptor? Shouldn't StaticFunctionAdaptors
have Functions as parameters?
I named the `auto_cast` function `explicitly` to do a explicit conversion.

Other uses of this pattern I've see are to create a buffer able to store
a specific struct

struct S;
S* ptr = create_msg();

create_msg returns an class that is convertible to a T* and that
allocates sizeof(T) bytes of memory.

Why this pattern respect the DRY principle (we don't repeat S), I
believe it abuses of implicit conversions.
Anyway, I wanted just to share another use case for implicit.

http://pfultz2.github.io/Fit/doc/html/include/boost/fit/indirect.html
Wondering if it could be worth adding some preconditions as I guess that
the function call

indirect(f)(xs...)

will be undefined if f is null

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/infix.html**
*

One of the asked infix operations is pow. However we would like to
evaluate from right to left and have the higher precedence than * x pow
ypow z===pow(x, pow(y,z))
x * ypow z===x * pow(y,z) x pow y* z===pow(x,y) * zWith the proposed
infix notation this should be written
x <pow> (y<pow> z)===x * pow(y,z) x * (y<pow> z)===x * pow(y,z) (x <pow>
y)* z===pow(x,y) * z
and the following would be surprising
x <pow> y<pow> z===pow(pow(x, y, z) x * y<pow> z===pow(x * y, z) x <pow>
y*z===pow(x, y * z) It would work better for swap or compare (proposed
operator <=>) as these functions are not associative. I believe a note
should prevent the users for such bad use cases, for which it is better
to don't define the infix operator.
*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/lazy.html* Note
p0356r1. It would be good to have a comparison in order either to
improve the proposal or your library.
*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/match.html*
Same comments than for fit::conditional. What are ex typo "Proposal for
C++ Proposal for"
Can more than 2 parameters be final function objects?
*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/mutable.html
*Example of bad usage would be welcome.**Why Fit uses always const?
**Why not use a reference to the function object instead? Is there a
safe way to use MutableFunctionObjects with Fit?
*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/partial.html*
Are there any constraints on the type of xs. Shouldn't them be
MoveConstructibles?
http://pfultz2.github.io/Fit/doc/html/include/boost/fit/pipable.html
Are there any constraints on the type of x. Shouldn't it be
MoveConstructible?
http://pfultz2.github.io/Fit/doc/html/include/boost/fit/protect.html It
is not clear to me what this useful for. Could you elaborate? Is there a
typo here
assert(lazy(f)(protect(lazy(g)(_1)))() == f(lazy(g)(_1))) ^ Shouldn't it
be assert(lazy(f)(protect(lazy(g)),_1)() == f(lazy(g)(_1)))

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/result.html***
This adaptor converts explicitly the result of the call to the type. Have you considered `convert_to`?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/reveal.html*

as_failures, with_failures, .... /adaptors/ don't appear on the index.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/reverse_fold.html*

reverse_fold is not right fold, as right fold is right associative, isn't it?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/rotate.html***
Could you show a concrete use case?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/static.html***

What are the differences of this adaptor and the macro STATIC_FUNCTION?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/unpack.html*

Why name it unpack instead of apply like in C++17?
Need to introduce the Sequence concept and how it is customized?
unpack_sequence? How unpack_sequence is used by unpack?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/unpack_sequence.html***
Why unpack_sequence, and not the tuple-like traits?
Is unpack_sequence defined if the class defines the tuple-like traits?
Aside the example, it is not defined what the user must define.
What the member function apply must return?
How the unpack_sequence parameter and the Sequence parameter of member function apply are related?
I don't understand

         template<class F, class Sequence>
         constexpr static auto apply(F&& f, Sequence&& s) BOOST_FIT_RETURNS
         (
             s(std::forward<F>(f))
         );
Is a Sequence a callable taking a Callable or there is a type here? Is there a missing return?

I suspect it should be

         template<class F, class Sequence>
         constexpr static auto apply(F&& f, Sequence&& s) BOOST_FIT_RETURNS
         (
             std::forward<F>(f)(s0, ,, sk);
         );
If this is correct, does it means that unpack just forwards to unpack_sequence?
Does the default specialization make use of INVOKE?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/capture.html***
Please, could you show how it is more flexible than lambda captures in C++?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/if.html*

What happens when the parameter is false_type? SFINAE?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/limit.html***
if a function object is overloaded with 2 and 3 parameters, would limit be useful or it is only useful when the number of parameters is fixed?
I believe this is answered in http://pfultz2.github.io/Fit/doc/html/doc/src/partialfunctions.html, however I believe it merits a clarification here.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/repeat.html*

Add requirement on the function parameter that must be a Unary function such that f(f(x)) is well formed.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/repeat_while.html*
The same here.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/arg.html***
What is the rational for the choice of been 1-based?
Could you add a rationale in the doc?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/construct.html*

Maybe add a reference to the factory proposal p03382.

What happens when the parameter is not a T is not constructible from the params? SFINAE?

What happens when the parameter is not a MetafunctionClass? SFINAE?

I believe Boost.Hana uses make instead of construct.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/decay.html*

Why not decay_copy?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/identity.html *is identity a function or a function object? do we need identity()(x)?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/placeholders.html***
Wondering if it is not better to locate them in a placeholders namespace, so that we can avoid possible collisions with other placeholders.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/placeholders.html#unamed-placeholder*

The same here

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/function_param_limit.html***
|function_param_limit is not a Metafunction. "|The|function_param_limit| metafunction ..."

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/is_callable.html***
F must be Callable can not be a requirement ;-)

Add reference to C++17 trait is_invocable.

Wondering if we should have it in type_traits, if not already there.

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/apply.html***
Confusion with std::apply. is this related to std::invoke?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/function.html*

"By default, all functions defined with|BOOST_FIT_STATIC_FUNCTION| use thereveal <http://pfultz2.github.io/Fit/doc/html/include/boost/fit/reveal.html> adaptor to improve error messages."

How to change this default?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/lift.html***
WhyBOOST_FIT_LIFT_CLASSdoesn't defines a instance as do
BOOST_FIT_STATIC_FUNCTION?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/pack.html***
How this is related with fit::capture?

*http://pfultz2.github.io/Fit/doc/html/include/boost/fit/tap.html*

How this is related to the tee unix command?

*http://pfultz2.github.io/Fit/doc/html/doc/src/configurations.html***Which ones?

Next message: Jason Roehm: "Re: [boost] [build] using nvcc as toolset target?"
Previous message: Richard Hodges: "Re: [boost] Review Request: impl_ptr (pimpl)"
Next in thread: P F: "Re: [boost] Fit review - viboes"
Reply: P F: "Re: [boost] Fit review - viboes"

Date view	Thread view	Subject view	Author view

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk