AMDG On 03/05/2016 07:21 PM, Vicente J. Botet Escriba wrote:

I want to start a new sub-thread about some of the concerns of Steven Watanabe about whether some of the contents of this library fits better in Boost.Config. In particular the file boost/fit/returns.hpp.

When I mentioned Boost.Config, I was talking about things like #ifndef BOOST_FIT_NO_EXPRESSION_SFINAE #ifdef _MSC_VER #define BOOST_FIT_NO_EXPRESSION_SFINAE 1 #else #define BOOST_FIT_NO_EXPRESSION_SFINAE 0 #endif #endif or #ifndef BOOST_FIT_HAS_TEMPLATE_ALIAS #if (defined(__GNUC__) && !defined (__clang__) && __GNUC__ == 4 && __GNUC_MINOR__ < 7) #define BOOST_FIT_HAS_TEMPLATE_ALIAS 0 #else #define BOOST_FIT_HAS_TEMPLATE_ALIAS 1 #endif #endif In Christ, Steven Watanabe

Le 06/03/2016 06:16, paul Fultz a écrit :

...

On Saturday, March 5, 2016 10:50 PM, Steven Watanabe <watanabesj@gmail.com> wrote:

...

AMDG On 03/05/2016 07:21 PM, Vicente J. Botet Escriba wrote: I want to start a new sub-thread about some of the concerns of Steven Watanabe about whether some of the contents of this library fits better in Boost.Config. In particular the file boost/fit/returns.hpp.

When I mentioned Boost.Config, I was talking about things like

#ifndef BOOST_FIT_NO_EXPRESSION_SFINAE #ifdef _MSC_VER #define BOOST_FIT_NO_EXPRESSION_SFINAE 1 #else #define BOOST_FIT_NO_EXPRESSION_SFINAE 0 #endif #endif This is can be configurable, whereas Boost.Config it is not.

I'm not sure this is true. http://www.boost.org/doc/libs/1_60_0/libs/config/doc/html/index.html#boost_c...

...

or

#ifndef BOOST_FIT_HAS_TEMPLATE_ALIAS #if (defined(__GNUC__) && !defined (__clang__) && __GNUC__ == 4 && __GNUC_MINOR__ < 7) #define BOOST_FIT_HAS_TEMPLATE_ALIAS 0 #else #define BOOST_FIT_HAS_TEMPLATE_ALIAS 1 #endif #endif

This could be replaced with Boost.Config, however, I have ran into problems before with template aliases on gcc 4.7, so I wanted to make this configurable as well, although it could fallback onto Boost.Config right now.

Detection and configuration are two different things. There are two libraries that do detection, Boost.Config or Boost.Predef. If for some reason you want to configure your library for a specific point this must be different from the detection of whether a feature is available or not. Of course the user could only disable an available feature, not the opposite. Paul you could use either Boost.Config or Boost.Predef for the detection part. I know less Boost.Predef. Anything that can be configured must be documented in your library. Both libraries have also a way to configure but in this case you configure the whole Boost library that use these libraries. It is worth noting that the Fit library don't depend today on any Boost library and that Paul has reinvented a lot of things Boost provides already. I can understand this approach for a library independent of Boost, but when the library in integrated in Boost, it should use other ad-hoc Boost libraries. Steven, this was my fault. I should have done a deeper code inspection before the review to request the adaptation. Apologies to all the reviewers for not have been done that. I believe that we can identify the parts that need to be reworked to adapt its style to the Boost. Boost Hana, was also almost in the same case and had also its own standard type traits to avoid dependencies. Fit is a C++11 library that want to support compilers that are not too C++11 compliant. I would prefer that the library was a C++14 library that supports only the last compilers, Tis will make the code more readable, but it is up to Paul to decide what he wants to support. Best, Vicente

AMDG On 03/06/2016 09:39 PM, Paul Fultz II wrote:

On Sunday, March 6, 2016 at 3:44:31 AM UTC-6, Vicente J. Botet Escriba wrote:

...

Le 06/03/2016 06:16, paul Fultz a écrit :

...

...

On Saturday, March 5, 2016 10:50 PM, Steven Watanabe <

watan...@gmail.com <javascript:>> wrote:

...

...

#ifndef BOOST_FIT_NO_EXPRESSION_SFINAE #ifdef _MSC_VER #define BOOST_FIT_NO_EXPRESSION_SFINAE 1 #else #define BOOST_FIT_NO_EXPRESSION_SFINAE 0 #endif #endif

This is can be configurable, whereas Boost.Config it is not. I'm not sure this is true.

http://www.boost.org/doc/libs/1_60_0/libs/config/doc/html/index.html#boost_c...

That doesn't seem easily configurable by the user. I think I would prefer to make it configurable by the library and use Boost.Config for the default value.

Why does it need to be easily configurable? No one is ever going to care about it except when Boost.Config is wrong. In Christ, Steven Watanabe

Le 13/03/2016 23:14, Lee Clagett a écrit :
> 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
>
>
Thanks Lee for your review.
>> 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?
Yes, this is a point that must be solved.
> - 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.
While doing the Hana review had a discussion and accepted the functional 
part in Hanna as we know that we would have Fit in Boost. The high order 
functions in Hana belong to a distinct library. Hana is a C++14 library. 
Fit is C++11.
>
>      Two of the reasons stated are constexpr and SFINAE support.
I don't believe we can do better with a C++11 compiler.
Please could you elaborate about SFINAE, what is the issue?
>      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.
IMO, it out of the scope of this review to discuss how other libraries 
will be updated.
> - 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?
I let Paul respond here.
> - 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).
Yes more documentation is always welcome. pipable and variadic functions 
or functions with default parameter don't works well :( and I don't know 
how it could work.
> - 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.
It is curious that there is already something ^like^ that in Boost :)
I like this infix notation. It is a toy. I could also live without.
> - 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?
Please, could you elaborate?
> - 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.
Good suggestion.
> - Alias
>      Why is this provided as a utility? What does this have to do with
>      functions? This seems like an implementation detail.
Agreed. This is out of the scope of the library and even useful should 
be moved away. As signaled by Paul, I have already something like that 
in a lot of my libraries. Why not Boost.Utility?
>
>
>>     * 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?
This is a detail of implementation. Please, could you tell us where this 
is use at the user level?
> - 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?
Please, could you give some concrete examples?
> - 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?
Paul?
> - 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!
Paul?
> - 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).
Paul?
> - 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?
There are not too much standard libraries that have included the forward 
constexpr. We do that usually.
> - 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.
There is already a Github issue accepted by Paul.
> - 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.
This is something a Boost library author decides. Of course there is not 
too much sense to don't support more recent compilers.
> - 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.
There are only 3 macros configuration documented. The others should be 
renamed BOOST_FIT_DETAIL.
Paul has accepted to use Boost.Config for the detection macros, but 
Boost.Config uses Boost.Core for his tests :(
There is already an Github issue that Paul must address.
>
>
>>     * 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.
Could you elaborate?
> - 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.
I believe the library must document whatever. Adding some links are 
always welcome.
The Callable concept seems the same even if expressed in a different way.
> - 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.
Agreed.
> - 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.
AFAIK, Fit has no specific extension mechanism. The user is free to 
define as many adaptors, functions as she wants.
I agree that some examples of how to implement one simple adaptor will 
be welcome.
>
>>     * 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.
Good point. Paul added bjam support just before the review. He don't 
master yet the basic of bJam.
>
>
>>     * 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?
I don't reach to understand why you are rejecting the library. Is 
because of the quality of the code or the test? The global design? the 
documentation? Is there something that must be modified so you could 
accept the library?
>
>
>> - 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.
I understand. The library is not so little.
>
> Lee
>
Thanks again,
Vicente

On Sunday, March 13, 2016 5:17 PM, Lee Clagett <forum@leeclagett.com> wrote:

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?

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

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

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

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

...

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

On Monday, March 14, 2016 11:15 PM, Lee Clagett <forum@leeclagett.com> wrote: paul Fultz <pfultz2@yahoo.com> wrote:

...

On Sunday, March 13, 2016 5:17 PM, Lee Clagett <forum@leeclagett.com> 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

...

On Tue, 15 Mar 2016 08:20:27 +0000 (UTC)

On Wednesday, March 16, 2016 11:54 PM, Lee Clagett <forum@leeclagett.com> wrote: paul Fultz <pfultz2@yahoo.com> wrote:

...

...

On Monday, March 14, 2016 11:15 PM, Lee Clagett <forum@leeclagett.com> wrote: [snip] 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.

The lazy approach is actually more flexible, I was being too difficult earlier. I was trying to go for consistency on the adapted calls, but it is not worth the loss in flexibility.

...

...

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

This was the case I referred to - a partially evaluated function that returned a pipe_closure into the flow adaptor. Although I did think of another interesting case:

struct foo { constexpr bool operator()(int, int, int) const { return true; } };

int main() { const auto bar = fit::pipable(foo{})(1); // bar is dead? return 0; }

`bar` doesn't appear to be invokable. fit::limit does not help here

either - since its an upward bound constraint ...?

Nope, limit doesn't help in this case. Limit is mainly for the case where you give it the exact number of parameters because you want it to be fully evaluated, that is if you write: struct C {}; auto bar = pipable(foo{})(C{}, C{}, C{}); So `foo` is not callable with `C`, however, this compiles as `bar` is really just a partially evaluate `foo`. So, if you want to present an error to the user, limit can be used: struct C {}; auto bar = pipable(limit_c<3>(foo{})(C{}, C{}, C{}); Of course, this can't handle the case of giving it less number of parameters.

A compiler error at> the creation of `bar` would have been preferred, but at least one is

given later in any attempt to call `operator()` on it.

Yes, I wish there was a way to present the error earlier, but I don't think its possible. I have thought about writing a `final` function that would give a compiler error if it was given a partially evaluted function, so then the user could write: auto bar = final(pipable(foo{})(1)); So there would be a compiler error immediately.

...

...

[snip]

...

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.

Consider a loop-based approach if you decide to have the last stage non-constexpr (it is currently _not_ constexpr). I do not see a point in making it recursive in that situation. Although, if you mark it constexpr, compilers that properly support C++11 constexpr recursion

will do more calls without having to manipulate the pre-defined limit.

True, for C++14 I could use a loop for constexpr as well.

...

[snip]

...

...

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.

This is because for a trait `T` is a better match than its base. But the base is definitely there:

namespace test { struct compress_; struct encrypt_; struct base64_;

template<typename> struct interface { using bytes = std::vector<std::uint8_t>;

bytes operator()(bytes const&) { return bytes{}; }; };

constexpr const interface<compress_> compress{}; constexpr const interface<encrypt_> encrypt{}; constexpr const interface<base64_> base64{}; constexpr const auto output = fit::flow(compress, encrypt, base64);

constexpr bool is_interface(...) { return false; }

template<typename T> constexpr bool is_interface(interface<T> const&) { return true; }

constexpr bool is_encrypt(...) { return false; } constexpr bool is_encrypt(interface<encrypt_> const&) { return true; } }

int main() { static_assert(test::is_interface(test::compress), "passed"); static_assert(test::is_interface(test::encrypt), "passed"); static_assert(test::is_interface(test::base64), "passed"); static_assert(test::is_interface(test::output), "passed");

static_assert(!test::is_encrypt(test::compress), "passed"); static_assert(test::is_encrypt(test::encrypt), "passed"); static_assert(!test::is_encrypt(test::base64), "passed"); static_assert(test::is_encrypt(test::output), "passed");

return 0; }

In this context it might make sense for `output` to be considered an encryption, compression, and base64 function, since it is composed of those parts, but does it always make sense? Is a function adapted by `fit::flip`, etc., always related to the original in academic OOP concepts and should it be related in the C++ type system? My example from Phoenix earlier was dismissed as being an issue with its design, but it cannot always be a flaw to have a callable with associated

functions?

However, if it doesn't make sense to override the class's behavior because of its associated functions then the class should be declared final.

Admittedly, I do need to think about this some more to know if there are other issues in common usage other than what Steven and I have pointed out.

...

[snip]

...

...

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.

The example was poor, because I think Phoenix became the focus. Several of the adaptors require template callables to be SFINAE friendly, or they will not work. This is an advanced topic, especially when basic template usage might be tried in a callable.

I cannot find any documentation mentioning the relationship to the conditional calls of Fit, and templates. Describing how SFINAE works is definitely out-of-scope. However, I think mentioning that any adaptor based on conditional calls must be SFINAE friendly when the adapted function is a templated callable would be a good start. Additionally, all adaptors that have conditional logic should be explicitly listed (somehow) to provide a reference back to the section describing conditional calling. Hopefully this conditional section will also have a discussion of variadics (both C and C++) and default arguments

because as I already mentioned that could surprise some programmers too.

This would be good to add the documentation.

...

...

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

If two functions are variadic, shouldn't it fail to compile? Or are you thinking of restricting the Fit placeholders to an upward bound on

arguments?

The issue is that the following function should not be callable(it is a compile error but not a substitution failure): auto f = (_1 - _2); static_assert(!is_callable<decltype(f), int>::value, "Callable"); That is if the user does not give of enough parameters for the function placeholders, then the function should not callable. The reason why its not in this case is because the function `arg` is not SFINAE-friendly when given out- of-bounds argument index, that is, `arg_c<2>(1)` should not be callable.

Lee