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

Subject: Re: [boost] [Fit] formal review starts today
From: paul Fultz (pfultz2_at_[hidden])
Date: 2016-03-13 21:47:21

On Sunday, March 13, 2016 5:17 PM, Lee Clagett <forum_at_[hidden]> wrote:

>
>
>On Thu, 3 Mar 2016 12:43:10 +0100
>"Vicente J. Botet Escriba" <vicente.botet_at_[hidden]> wrote:
>
>> Dear Boost community, sorry for the late anounce
>> 
>> The formal review of Paul Fultz II's Fit library starts today, 2nd
>> March and ends on 13th March.
>> 
>> Fit is a header-only C++11/C++14 library that provides utilities for
>> functions and function objects.
>> 
>[snip]
>> 
>> We encourage your participation in this review. At a minimum, kindly
>> state:
>> - Whether you believe the library should be accepted into Boost
>>    * Conditions for acceptance
>
>I do not think the library should be accepted at this time.
>
>
>> - Your name
>
>Lee Clagett
>
>
>> - Your knowledge of the problem domain.
>
>I have used and grokked the boost::phoenix implementation extensively.
>I do not have many other qualification in this area.
>
>
>> You are strongly encouraged to also provide additional information:
>> - What is your evaluation of the library's:
>>    * Design
>
>- Gcc 5.0+
>    Why are the newer compiler versions not supported? I thought C++11

>    support been improving in Gcc?

Gcc 5.1 is the only one compiler that will never be supported as it has way
too many regressions. The tests may very well pass with Gcc 5.2 or 5.3. They
have in the past. However, I don't have the testing infrastructure in place to
test Gcc 5.2 or 5.3 on a regular basis, so I can't declare it officially
supported.

>- Functions section and fit::lazy adaptor
>    I think everything in this section should be removed from the
>    library. I do not see a need for duplication with other existing

>    Boost lambda libraries 

Other boost lambda libraries do not support constexpr and are not compatible
with std::bind.

>    and even Hana. 

And Hana is currently not portable.

>    I think this library should>    focus solely on composing functions in different ways. However, I
>    do not think the inclusion of this functionality is a reason to
>    reject the library.
>
>    Two of the reasons stated are constexpr and SFINAE support.
>    Could existing lambda libraries could be upgraded? Hana should be

>    upgradeable at a minimum. 

Louis has made it very clear that he won't support non-C++14 compiler(which I
think is fine for Hana).

>    *And* Thomas Heller was floating around a
>    C++14 phoenix that was also interesting.

And while he is floating around that idea people can use fit::lazy.

>- fit::lazy
>    Why is the creation of a  bound function a two-step process?
>        fit::lazy(foo)(_1, 2)
>    Why differ from existing libraries where a single function call

>    creates the binding?

This is to make the interface consistent with the rest of the library.
Adaptors take functions as their parameters. It could be written as a
decorator, which would look like this:

fit::lazy(_1, 2)(foo);

However, I think that is strange. 
>- Pipable Adaptor
>    This implicitly adds an additional `operator()` to the function
>    which acts as a partial function adaptor (or something similar).

>    Would it better to add a specially named function for this? 

No, this what allows pipable functions to be used in fit::flow, which is an
alternative to invoking the functions without using the | operator, as some
don't like the idea of overloading operators in this way.

>    The>    selection to choose immediate vs partial evaluation is done by
>    SFINAE, which creates some interesting effects with `operator()`
>    overloads or default arguments:
>
>      struct default_sum_ {
>        int operator()(int left, int right = 0) const {
>          return left + right;
>        }
>      };
>
>      FIT_STATIC_FUNCTION(default_sum) = fit::pipable(default_sum_{});
>
>      int main() {
>        // prints 1 (expected)
>        std::cout << default_sum(1) << std::endl;
>
>        // prints 1 (expected)
>        std::cout << (1 | default_sum()) << std::endl;
>
>        // prints 1 (bitwise-or unexpected?)
>        std::cout << (1 | default_sum(1)) << std::endl;
>        return 0;
>      }
>
>    Or perhaps a mention in the documentation about how this adaptor

>    _really_ works (immediate vs partial evaluation).

This is a good point and should be documented.

>- Infix operator
>    Steve Watanabe already brought this up (and theres a corresponding
>    github issue [0]); I would suggest inclusion into boost _without_
>    this feature. In addition to potential operator precedence issues,
>    this is used in a context where `<IDENTIFIER>` is also used for
>    class and function template instantiations.
>
>        template<unsigned V>
>        using unsigned_constant = std::integral_contant<unsigned, V>;
>
>        unsigned_constant<10>::value <plus> unsigned_constant<11>::value
>
>    (To me) this makes the first `value` look like a nested typename or
>    a nested template function with `plus` being a template argument. Of
>    course, the AST _must_ be one with `operator<` because
>    neither `typename` nor `template` precedes the token. Both grammar
>    ambiguities already confuse programmers, will this exacerbate the
>    problem? OTOH, it should be obvious after the `<plus>` that this
>    must be an `operator>` since right side is not valid for a class or
>    function instantiation.
>- fit::detail::make and fit::construct
>    Aren't these doing the same thing? `detail::make` is slightly more
>    "stripped" down, but is it necessary? Also, should this be
>    exposed/documented to users who wish to extend the library in some

>    way?

detail::make is used to declare the functions for the adaptors. It might be possible to merge this in the future as the default behavior for fit::construct will become by value.
>- Placeholders
>    If they were put in their own namespace, all of them could be
>    brought into the current namespace with a single `using` without

>    bringing in the remainder of the Fit library. Phoenix does this.

This is a good idea. I can do that.

>- Alias
>    Why is this provided as a utility? What does this have to do with

>    functions? This seems like an implementation detail.

It doesn't have to do with functions. However, I do use this in other
libraries as well. I may as well leave it undocumented for now.
>
>
>>    * Implementation
>
>- fit::detail::compressed_pair
>    - Why does it _always_ inherit from the types, even when they are
>      not empty? Duplicate types are handled with a tag (so no
>      compiler error), but every other implementation I've seen uses
>      composition except for EBCO cases. Luckily it does not appear that

>      this has issues since it is an implementation detail.

So if one of the classes are not empty use composition. This could be a
possibility. I just followed the logic I used for pack. Checking if one class
in pack is not empty, and changing to no inheritance, seems it would be a
little expensive at compile time.
>    - Why do the functions `first`, `second` take parameter packs?
>      Retrieving the value stored in the pair does not require any
>      values, and the parameter pack eventually is dropped in
>      `alias_value`. I think this makes the code unnecessarily

>      confusing.

The class is still an incomplete type when used inside of a decltype and
constexpr causes the compiler to eargerly evaluate this. So the extra packs
make the function depend on the template parameter so it won't be evaluated
until instatiated by a template, which the class will be complete at that
point.
>    - Why does the function get_base exist as a public function? This
>      library uses inheritance liberally, so I suppose its easier to

>      retrieve one if its bases?

Yes it makes it easier to retrieve the base function.

>- Forwarded Parameter Packs
>    In several places it appears that parameter packs are being
>    passed to one function and forwarded to another function in the same
>    expression. It should be implementation defined as to whether the
>    object has been moved-from or not, although in every case it
>    appears the parameter pack is unused in the non-forwarded case. So

>    why is the parameter pack used?

The same as I explained above.

>- fit::detail::static_default_function
>    I see this templated struct forward declared in `infix.hpp` and
>    `pipable.hpp` with operator overloads for `|`, `<`, and `>`, but I
>    see no definition. Is this cruft? Was BOOST_FIT_STATIC_FUNCTION

>    supposed to use this in some way?

I used to have a static_default_function in the library, that needs to be
removed.
>- fit::pipable Inconsistencies
>    Using the lambda declaration macros or the StaticFunctionAdaptor
>    `fit::static_`  will automatically provide operator overloads for
>    `|` iff the proper header file is included. But using the function
>    declaration macros, the operators are only overloaded if the

>    `pipable` adaptor is used. Was this intended? 

No, I should add a more general way of adding the pipable operators to
functions.

> Any automatic>    inclusions of operator overloads should be mentioned in the
>    documentation. Also, the `operator|` overload for `static_` is
>    SFINAE'ed away with an extra `base_function()` call, which does not
>    appear to be intended because the lambda overload has no such
>    feature. Should support for these types even be automatically
>    included? The partial-like feature of `pipable_adaptor` is not
>    provided for lambdas or `static_` either. To clarify the above:
>
>      struct sqr_ {
>        int operator()(int value) const {
>          return value * value;
>        }
>      };
>
>      FIT_STATIC_FUNCTION(sqr) = sqr_{};
>      FIT_STATIC_FUNCTION(pipe_sqr) = fit::pipable(sqr);
>      FIT_STATIC_FUNCTION(static_sqr) = fit::static_<sqr_>{};
>      constexpr const auto lambda_sqr = FIT_STATIC_LAMBDA(int value) {
>        return value * value;
>      };
>
>      int main() {
>        // DNC == does not compile
>
>        std::cout << sqr(3) << std::endl; // outputs 9
>        // std::cout << (2 | sqr()) << std::endl; // DNC
>        // std::cout << (2 | sqr) << std::endl; // DNC
>        std::cout << pipe_sqr(3) << std::endl; // outputs 9
>        std::cout << (3 | pipe_sqr()) << std::endl; // outputs 9
>        std::cout << (3 | pipe_sqr) << std::endl; // outputs 9
>        std::cout << static_sqr(3) << std::endl; // outputs 9
>        // std::cout << (3 | static_sqr()) << std::endl; // DNC
>        // std::cout << (3 | static_sqr) << std::endl; // DNC
>        std::cout << lambda_sqr(3) << std::endl; // outputs 9
>        // std::cout << (3 | lambda_sqr()) << std::endl; // DNC
>        std::cout << (3 | lambda_sqr) << std::endl; // outputs 9
>        return 0;
>      }
>
>    I do not see a consistent pattern, especially with the lambda and
>    static_ adaptor. And document whatever is done!
>- boost::fit::repeat
>    The implementation allows for non-IntegralConstant types (anything
>    that doesn't have a nested ::type::value). This then defers to an
>    alternate version which is based on a template loop with a fixed
>    depth recursion unrolling. Except the 0th implementation is a
>    recursive call to the same function so:
>
>        // outputs 17, one-past the recursion depth
>        std::cout << 
>          fit::repeat(FIT_RECURSIVE_CONSTEXPR_DEPTH + 1)
>            (fit::_1 + 1)(0) << 
>          std::endl;
>

>    Why is there a fixed unrolling internally? 

This is to allow recursion when using constexpr. If the unrolling wasn't
there, the compiler would reach its internal limit for constexpr depth(due to
infinite constexpr instantiations). So, the unrolling is constexpr, but when
it switches to the recursive version it is no longer using constexpr, which
causes the compiler to stop constexpr instantiations. So,
FIT_RECURSIVE_CONSTEXPR_DEPTH limits the depth for constexpr, however, the
runtime version(when used in a non-constexpr context) does not have this
limitation(except for stack size). Both fit::repeat_while and fit::fix use a
similar technique.

>  Performance reasons? Why>    not restrict to only compile time values or have a stack based
>    loop for this version? The recursive implementation can cause
>    stackoverflows, which is easy to do if repeat is given a runtime
>    value (I verified the stackoverflow case with int::max above).
>- boost::fit::forward
>    Does this need to be in the library? C++14 updates the function to
>    be constexpr too, and `static_cast<T&&>` can be done in rare cases
>    where someone needs constexpr forwarding in C++11. Furthermore,
>    should the macro _always_ expand to `static_cast<T&&>`?. Louis Donne
>    did some analysis showing that this reduced compile time since each
>    unique invocation of `std::forward` required a template
>    instantiation. What is the advantage of conditionally calling

>    `boost::forward` based on compiler version?

Gcc 4.6 and MSVC have trouble grokking the static_cast sometimes. So that's
why fit::forward is used instead.
>- Inheritance issues
>    +1 Steven Watanabe's mentioned of this. I only see disadvantages
>    with inheriting from user callables:
>
>      namespace arg = boost::phoenix::placeholders;
>
>      // outputs 3
>      std::cout << fit::flip(arg::_1 - arg::_2)(3, 6) << std::endl;
>
>      // outputs -9 (the parameters are not flipped!)
>      std::cout << 
>        (fit::flip(arg::_1 - arg::_2) * arg::_1)(3, 6) <<
>        std::endl;
>
>    Operators from phoenix are still in the set of candidate functions

>    due to inheritance, and this does not seem correct.

It seems like the operators are not properly constrained.

>- Macro specializations
>    There are already a lot of specializations for various compiler
>    versions that do not fully implement C++11. I think the fit
>    implementation would be more clear if support for these

>    compilers was removed or at least reduced. 

I would like a large amount of portability.

> For example, there are a>    number of specializations to support Gcc 4.6 specifically, but yet

>    Gcc 5.0+ is not supported.

Not true. Gcc 5.1 is not supported, does not mean that gcc 5.0+ is not.

>- Config file
>    Macro switches are scattered throughout various files, and I
>    typically had to grep for them. And then often that switch was
>    dependent on some switch defined in yet another file. Could the
>    defines unique to Fit be placed in a config header? And Fit would

>    hopefully use the Boost config where possible.

That would be a good idea.

>
>
>>    * Documentation
>
>- IntegralConstant 

>    Needs to be defined in the Concept section.

Alright, will add it.

>- fit::static_
>    I think it will have runtime thread synchronization just to access
>    the callable. It would be nice if there was a way around this, but
>    there probably isn't one. I think the documentation should at least
>    mention that constexpr capable functions should avoid this adaptor
>    for performance reasons, and then reference one of the other

>    adaptors.

It already says, "Functions initialized by `static_` cannot be used in
`constexpr` functions.", and explains what should be done instead. So I should
make it clearer that constexpr is not supported wiht `static_`.
>- fit::is_callable
>    Is this trait simply an alias for std::is_callable, or the Callable

>    concept defined by Fit? 

I don't think it is an alias for std::is_callable, because I think it works
like std::is_callable<F(T..)> whereas in Fit, it works like
fit::is_callable<F, T...>. I don't support the function signature because it
is problematic and unnecessary. 

And Callable should be the same definition as in the standard.

> I _believe_ they are identical, but C++17>    hasn't been finalized (right?), and/or what if they diverge? I
>    think its worth linking the fit::is_callable documentation to the
>    Callable concept you have defined by the Fit documentation,

>    although I am not sure of this is typical boost behavior.

That is good idea to link Callable and is_callable.

>- fit::partial
>    I think its worth mentioning the behavior with optional arguments
>    (default or variadic). When the function underlying function is

>    actually invoked depends on this.

Yes

>- fit::limit
>    Does the implementation internally use the annotation? What

>    advantage is there to this and the associated metafunction?

This can be used with fit::pipable and fit::partial to improve the errors. I
also make the limit publicly available through the metafunction so other users
can take advantage of the annotation in a similar fashion.
>- fit::tap

>    Is `tee` more appropriate?

Perhaps, I got the name tap from underscore.js.

>- Extending Library
>    I think it might be useful to discuss how to write your own adaptor

>    or decorator.

This is a good point. I would like to expand on that at some point.

>
>
>>    * Tests
>
>It would be nice if some other boost function-like libraries are used
>in testing to ensure compatibility (optionally added when boost is
>detected / enabled). Bind, phoenix, hana seem like good candidates,
>and perhaps one of the other lambda libraries.
>
>Also what about testing expected compile-failures? bjam has a
>compile-fail test option, and I think Cmake can be abused to do the

>same.

Yes, I'll try to add some compile-time failures.

>
>
>>    * Usefulness
>
>I think the library has merit and would be useful to the community. My
>reason for recommending rejection is based mainly on the idea that
>Boost should become a collection of highly polished libraries since
>github publishing is becoming so common. The review manager should take

>this into account when considering my review; 

I don't understand this point.

> I think Paul is capable>of addressing my concerns. I also like Louis' comment about justifying
>the purpose of the library - or at least provide a better definition
>for the library. And how much of the functionality can already be done

>with Hana?

I think at one point Hana was considering using Fit library. However, there
are differences. Fit is SFINAE-friendly, space-optimized, lazy evaluated, and
portable, to name a few key differences. Fit also has a much smaller scope
than Hana. I can go on into more detail about these differences if you like.
>
>
>> - Did you attempt to use the library? If so:
>>    * Which compiler(s)
>
>I used Clang 3.4 and Gcc 4.8 in a Linux environment. I primarily tested
>several edge cases that I saw while reading the code.
>
>
>>    * What was the experience? Any problems?
>
>Any problems were previously noted.
>
>
>> - How much effort did you put into your evaluation of the review?
>
>I read a signicant portion of the code, so I did a fairly thorough

>review. Did not read _all_ of the code though.

And thanks Lee for your review.

>
>
>Lee
>
>_______________________________________________
>Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>
>
>
>