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

Subject: Re: [boost] [Fit] formal review starts today
From: paul Fultz (pfultz2_at_[hidden])
Date: 2016-03-15 04:20:27

> On Monday, March 14, 2016 11:15 PM, Lee Clagett <forum_at_[hidden]> wrote:
> > On Mon, 14 Mar 2016 01:47:21 +0000 (UTC)
> paul Fultz <pfultz2_at_[hidden]> wrote:
>>  On Sunday, March 13, 2016 5:17 PM, Lee Clagett <forum_at_[hidden]>
>>  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.

But adaptors take functions as a their parameters. If there are other
parameters than just functions, than a decorator is used. 

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

Yes, instead of writing:

auto r = x | f(y) | g(z);

The user can write:

auto r = flow(f(y), g(z))(x);

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

Yes, that is correct, which is unfortunate. 

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

Hmm, that may work. I originally tried a two-phase approach at first, which
didn't work. However, it may work, at least for fit::repeat and
fit::repeat_while. I don't think it will work at all for fit::fix even with an
explicit return type. I will need to investigate this more.

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

Yes, it is. I need to look into a way to prevent or minimize this.

> - is it worth mentioning the
> limitation in the documentation?

Yes, I do mention the limitation in fit::fix, but it should be mention with
fit::repeat as well.
> 
>>  >  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?  
>> 
>>  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 ...

Yep they do. One issue with gcc 4.6 is that it can't mangle a static_cast, and
MSVC has its own funkiness going on.

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

And thats the problem right there. It shouldn't still be a phoenix actor after
inheritance. Using fit::lazy, it becomes a bind expression, however, after
using flip, this is no longer the case:

auto lazy_sum = lazy(_+_)(_1, _2);
auto f = flip(lazy_sum);

static_assert(std::is_bind_expression<decltype(lazy_sum)>::value, "A bind expression");
static_assert(!std::is_bind_expression<decltype(f)>::value, "Not a bind expression");

Furthermore, your example doesn't compile when using Fit's placeholders.

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

Thats something, I will note in the documention then.

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

Of course more work will be needed for final 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;

And that hard error comes from phoenix, of course, as it does compile using the Fit
placeholders:

const auto sub = (_1 - _2);
std::cout << 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;

And that works with Fit placeholders.

> 
>     // does not compile, ambiguous (both have variadic overloads)

>     // std::cout << fit::match(negate, sub)(0, 1) << std::endl;

This does not, and is a bug. I am going to look into that.

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

Yes, fit::conditional is meant to be "lazy" in instantiating the functions. In
fact, the library relies on this internally. I will make sure to document this
to make that clearer.

> Or should
> user code always strive to be SFINAE friendly? 

Yes, of course. If the user doesn't properly constrain their functions, how
can they expect the library to detect the callability of the function?

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