On Thu, 3 Mar 2016 12:43:10 +0100 "Vicente J. Botet Escriba" <vicente.botet@wanadoo.fr> 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