Re: [Boost-users] [boost] [Fit] formal review starts today

On Sunday, March 13, 2016 5:17 PM, Lee Clagett wrote:

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

...

- 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_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_{}; 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` 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?

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 whereas in Fit, it works like fit::is_callable. 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