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

Le 13/03/2016 23:14, Lee Clagett a écrit :
> On Thu, 3 Mar 2016 12:43:10 +0100
> "Vicente J. Botet Escriba"  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
>
>
Thanks Lee for your review.
>> 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?
Yes, this is a point that must be solved.
> - 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.
While doing the Hana review had a discussion and accepted the functional 
part in Hanna as we know that we would have Fit in Boost. The high order 
functions in Hana belong to a distinct library. Hana is a C++14 library. 
Fit is C++11.
>
>      Two of the reasons stated are constexpr and SFINAE support.
I don't believe we can do better with a C++11 compiler.
Please could you elaborate about SFINAE, what is the issue?
>      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.
IMO, it out of the scope of this review to discuss how other libraries 
will be updated.
> - 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?
I let Paul respond here.
> - 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).
Yes more documentation is always welcome. pipable and variadic functions 
or functions with default parameter don't works well :( and I don't know 
how it could work.
> - 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 `` is also used for
>      class and function template instantiations.
>
>          template
>          using unsigned_constant = std::integral_contant;
>
>          unsigned_constant<10>::value  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 `` that this
>      must be an `operator>` since right side is not valid for a class or
>      function instantiation.
It is curious that there is already something ^like^ that in Boost :)
I like this infix notation. It is a toy. I could also live without.
> - 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?
Please, could you elaborate?
> - 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.
Good suggestion.
> - Alias
>      Why is this provided as a utility? What does this have to do with
>      functions? This seems like an implementation detail.
Agreed. This is out of the scope of the library and even useful should 
be moved away. As signaled by Paul, I have already something like that 
in a lot of my libraries. Why not Boost.Utility?
>
>
>>     * 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?
This is a detail of implementation. Please, could you tell us where this 
is use at the user level?
> - 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?
Please, could you give some concrete examples?
> - 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?
Paul?
> - 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_{};
>        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!
Paul?
> - 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).
Paul?
> - boost::fit::forward
>      Does this need to be in the library? C++14 updates the function to
>      be constexpr too, and `static_cast` can be done in rare cases
>      where someone needs constexpr forwarding in C++11. Furthermore,
>      should the macro _always_ expand to `static_cast`?. 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?
There are not too much standard libraries that have included the forward 
constexpr. We do that usually.
> - 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.
There is already a Github issue accepted by Paul.
> - 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.
This is something a Boost library author decides. Of course there is not 
too much sense to don't support more recent compilers.
> - 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.
There are only 3 macros configuration documented. The others should be 
renamed BOOST_FIT_DETAIL.
Paul has accepted to use Boost.Config for the detection macros, but 
Boost.Config uses Boost.Core for his tests :(
There is already an Github issue that Paul must address.
>
>
>>     * 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.
Could you elaborate?
> - 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.
I believe the library must document whatever. Adding some links are 
always welcome.
The Callable concept seems the same even if expressed in a different way.
> - 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.
Agreed.
> - 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.
AFAIK, Fit has no specific extension mechanism. The user is free to 
define as many adaptors, functions as she wants.
I agree that some examples of how to implement one simple adaptor will 
be welcome.
>
>>     * 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.
Good point. Paul added bjam support just before the review. He don't 
master yet the basic of bJam.
>
>
>>     * 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?
I don't reach to understand why you are rejecting the library. Is 
because of the quality of the code or the test? The global design? the 
documentation? Is there something that must be modified so you could 
accept the library?
>
>
>> - 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.
I understand. The library is not so little.
>
> Lee
>
Thanks again,
Vicente