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

Subject: Re: [boost] [Fit] formal review starts today
From: Lee Clagett (forum_at_[hidden])
Date: 2016-03-15 00:14:21

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.

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

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