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

On Mon, 14 Mar 2016 01:47:21 +0000 (UTC) paul Fultz wrote:

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

...

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

FWIW, I am starting to like the lambda functionality in Fit.

...

- 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);

Most of the adaptors return a callable that is ready-for-use, whereas the lazy and decorator adaptors have not finished configuration. The partial and pipable adaptors have a third characteristic in that they _may_ be ready-for-use, depending on whether the function can be invoked. In this particular case, I was suggesting `fit::lazy(foo, _1, 2)` simply because it would return a callable that was ready-for-use, which I thought was more consistent with the other adaptors.

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.

You can use pipable operators in fit::flow? Everything returns an immediate value except for when a pipable-adapted function is partially-invoked, which returns a `pipe_closure` function. If the `operator|` is used at all a value is returned (which could be, but is unlikely to be another function). Can you give an example?

[snip]

...

...

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

Is this a problem in conjunction with the conditional checks? A "SFINAE expression check" would instantiate a constexpr function that was referenced in the trailing return, causing a compile failure ... something like that? Rather unfortunate, makes the code is a bit harder to read IMO. Not terrible. [snip]

...

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

I forgot about the return type compution of Fit - constexpr does not appear to be the issue in this case. The problem is asking the compiler to compute the return type of a recursive function. The compiler cannot "see" the depth unless it runs constexpr simultaneously while computing - and even then it would have to do a dead-code optimization in the ternary operator or something. Head-hurting for sure. I added constexpr to repeat_integral_decorator<0>::operator(), and defined FIT_RECURSIVE_CONSTEXPR_DEPTH=0 then ran test/repeat.cpp with Clang 3.4 and Gcc 4.8 both of which compiled just fine. Only a two-phase approach is needed here (DEPTH=1), where the first phase has the full expression for SFINAE purposes and normalizing the value with the ternary operator, and the second phase is a fixed return type based on the forwarded value. Is there a bug/issue in an older Gcc, Clang, or even MSVC that needed this larger loop unrolling? It is still unfortunate that a stackoverlow is possible with this implementation - is it worth mentioning the limitation in the documentation?

...

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.

They have trouble grokking the static_cast in certain situations? Because the forward function still has the same static_cast internally ...

...

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

The adapted flip function _is a_ phoenix actor due to inheritance. All Fit adapted functions have this anti-feature. Or at least I think its an anti-feature. [snip]

...

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

I meant that invoking a function adapted by `static_` could (and likely will) be slightly slower due to the usage of the function local static. The compiler/linker has to initialize the static before first use, which is allowed to occur before main. So either the compiler determines the global order of initialization (and initialize statics that are optionally used), or it generates initialization in a thread-safe way. I just disassembled code from Gcc 4.8 -O2/-O3 and Clang 3.4 -O2/-O3 on Linux, and a double-check lock scheme is used even if the `this` variable is *not* used. So avoiding this adaptor would be preferred for potential performance reasons, which I think is worth a documentation note. [snip]

...

...

* 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 thought more work was needed before inclusion. A few implementation details (mainly inheritance usage and the iffy reinterpret_case for lambda), and some work on documentation/design. If the documentation had recommendations for some edge cases, I would not have to guess whether they were considered during development. For instance, should there be some documentation explaining how the library "tests" for `operator()` in certain adaptors, and how templates (SFINAE/hard errrors) play a role: int main() { namespace arg = boost::phoenix::placeholders; const auto negate = (!arg::_1); const auto sub = (arg::_1 - arg::_2); // does not compile, hard-error with one argument call // std::cout << fit::partial(sub)(0)(1) << std::endl; // outputs -1 std::cout << fit::partial(sub)(0, 1) << std::endl; // outputs 1 std::cout << fit::conditional(negate, sub)(0) << std::endl; // outputs 1 std::cout << fit::conditional(negate, sub)(0, 1) << std::endl; // does not compile, hard-error with one argument call - // the ambiguous overload is irrelevant to the error // std::cout << fit::match(negate, sub)(0) << std::endl; // does not compile, ambiguous (both have variadic overloads) // std::cout << fit::match(negate, sub)(0, 1) << std::endl; return 0; } Even if Phoenix is old news with Fit, these types of errors will show up in user code. Can a user assume that conditional will never instantiate the next `operator()` if the previous succeeded? Or should user code always strive to be SFINAE friendly? When does it not matter? Hopefully I am not being too difficult here ...

...

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.

It was wrong of me to mention Hana again. The only overlap that I was thinking of at the time was the lambda library. It bothers me that Boost is maintaining Bind, Phoenix, and Lambda ... now Hana and possibly Fit with some overlap in this specific domain. Although Hana and Fit are much smaller in scope compared to the other Boost libraries in this domain. Lee