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

Subject: Re: [boost] [Fit] formal review starts today
From: Lee Clagett (forum_at_[hidden])
Date: 2016-03-13 18:14:44

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?
- 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 and even Hana. 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. *And* Thomas Heller was floating around a
    C++14 phoenix that was also interesting.
- 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?
- 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? 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).
- 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?
- 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.
- Alias
    Why is this provided as a utility? What does this have to do with
    functions? This seems like an implementation detail.

>    * 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.
    - 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.
    - 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?
- 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?
- 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?
- 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? 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? 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?
- 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.
- 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. For example, there are a
    number of specializations to support Gcc 4.6 specifically, but yet
    Gcc 5.0+ is not supported.
- 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.

>    * Documentation

- IntegralConstant 
    Needs to be defined in the Concept section.
- 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.
- fit::is_callable
    Is this trait simply an alias for std::is_callable, or the Callable
    concept defined by Fit? 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.
- 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.
- fit::limit
    Does the implementation internally use the annotation? What
    advantage is there to this and the associated metafunction?
- fit::tap
    Is `tee` more appropriate?
- Extending Library
    I think it might be useful to discuss how to write your own adaptor
    or decorator.

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

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

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

Lee