[Fit] formal review starts today
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. Fit is: - Modern: Fit takes advantages of modern C++11/C++14 features. It support both `constexpr` initialization and `constexpr` evaluation of functions. It takes advantage of type deduction, varidiac templates, and perfect forwarding to provide a simple and modern interface. - Relevant: Fit provides utilities for functions and does not try to implement a functional language in C++. As such, Fit solves many problems relevant to C++ programmers, including initialization of function objects and lambdas, overloading with ordering, improved return type deduction, and much more. - Lightweight: Fit builds simple lightweight abstraction on top of function objects. It does not require subscribing to an entire framework. Just use the parts you need. Fit is divided into three components: * Function Adaptors and Decorators: These enhance functions with additional capability. * Functions: These return functions that achieve a specific purpose. * Utilities: These are general utilities that are useful when defining or using functions Fit has been tested on gcc 4.6-4.9, clang 3.4-3.7, and Visual Studio 2015. For more information see: Github:https://github.com/pfultz2/Fit/tree/boost Documentation:http://pfultz2.github.io/Fit/doc/html/ 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 - Your name - Your knowledge of the problem domain. You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems? - How much effort did you put into your evaluation of the review? More information about the review process can be found here:http://www.boost.org/community/reviews.html We await your feedback! Best regards, Vicente J. Botet Escriba
AMDG On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late announce
The formal review of Paul Fultz II's Fit library starts today, 2nd March and ends on 13th March.
We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost
No, this library should not be accepted into Boost at this time.
* Conditions for acceptance - Your name
Steven Watanabe
- Your knowledge of the problem domain.
High.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems?
msvc-12 (failed), msvc-14, clang-3.3 (failed), gcc-4.8. This is basically as expected.
- How much effort did you put into your evaluation of the review?
~5 hours. I only finished half a dozen files, but I've seen enough problems that I feel justified in stopping now. Critical points: - Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref. - The library reinvents (poorly) its own versions of several components: compressed_pair <-> boost::compressed_pair, placeholders <-> lambda, bind, phoenix Various configuration macros <-> Boost.Config - The library attempts to take advantage of EBO, but makes several mistakes. (See attached tests which fail to compile) - Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable. It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible. Even worse are the functions (e.g. capture) which do not specify whether they require const or not. You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly. - The implementation has various useless bits, which serve no purpose that I can discern. - Please be pedantic about ADL. If you aren't it /will/ bite you. Details: Please add .gitattributes test/Jamfile.v2: - project fit Project names must be globally unique, so it's better to use /boost/fit (or leave out the name entirely) - [ test_all r ] "r" is completely meaningless here. - Actually, everything from rule test-all down can be reduced to: for local fileb in [ glob *.cpp ] { run $(fileb) ; } You don't need to create a special target to group the tests, unless you want more granularity than "run everything" or "run specific tests by name." alias.hpp: 57: #pragma warning(disable: 4579) Library headers *must* disable warnings inside push/pop. 67: std::false_type is defined somewhere... (It looks like you get <type_traits> via delegate.hpp) 74: Ditto for std::is_same 93: BOOST_FIT_UNARY_PERFECT_ID macro is undocumented and unused. 107: I have no idea why you made alias_value variadic. The extra arguments are unused, their presence is undocumented, and all calls from inside the library (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) that use them, could just as well be written without. 117: struct alias_inherit #if (defined(__GNUC__) && !defined (__clang__)) : std::conditional<(std::is_class<T>::value), T, alias<T>>::type Really? alias_inherit is documented to use inheritence, so it should be an error to use it with a non-class type. 145: // Since we disable the error for 4579 on MSVC, which leaves the static // member unitialized at runtime, it is, therefore, only safe to use this // class on types that are empty with constructors that have no possible // side effects. Shouldn't it also be safe for types with trivial default constructors, since then, zero-initialization will do the right thing? always.hpp: 22: "The `always_ref` version will return a reference, and it requires the value passed in to be an lvalue." The first time I read this I thought it meant that the function object created by always_ref would be a reference, not that it produces a reference when called. Consider rephrasing the sentence to avoid the ambiguity. 39: "Requirements: T must be: * CopyConstructible" This only applies to always, not always_ref, no? 61: #define BOOST_FIT_NO_CONSTEXPR_VOID 1 #ifndef BOOST_FIT_NO_CONSTEXPR_VOID ??? 121: always_base has boost::fit::detail as an associated namespace. 124: constexpr detail::always_base<void> operator()() const This overload is not documented. Should it be? apply.hpp: 29: assert(compress(apply, f)(x, y, z) == f(x)(y)(z)); I don't think that compress should be part of the semantics of apply. This belongs as an example. msvc-14.0 says: boost/fit/apply.hpp(83): warning C4003: not enough actual parameters for macro 'BOOST_FIT_APPLY_MEM_FN_CALL' apply_eval.hpp: 18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it. 90: return eval_ordered<R>(f, pack_join(...), ...) Watch out for ADL. 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested. arg.hpp: 29: /// template<class IntegralConstant> /// constexpr auto arg(IntegralConstant); /// /// template<std::size_t N, class... Ts> /// constexpr auto arg_c(Ts&&...); arg and argc should do the same thing. It looks like arg creates a function object, while arg_c evaluates it directly. 86: make_args_at(...)(..., make_perfect_ref(...)...) ADL... 95: get_args<N>(...) ADL by.hpp: 8: BOOST_FIT_GUARD_FUNCTION_ON_H The #include guard doesn't match the file name 22: "Also, if just a projection is given, then the projection will be called for each of its arguments." I don't see any good reason for this to be an overload of by. It's essentially a for_each. 25: Note: All projections are always evaluated in order from left-to-right. By only takes one projection. This is a lot of code to implement what is essentially a one-liner: apply_eval(f, capture_forward(x)(p)...) 135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow You should really have a single BOOST_FIT_VOID_RETURN and use it consistently everywhere. capture.hpp: 32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so. 79: return always_ref(*this)(xs...); Why not return *this? The always_ref dance serves no purpose, other than making the code hard to follow. placeholders.hpp: We really don't need another lambda library, let alone a half-baked one. concepts.md: Please reference the C++ standard when defining INVOKE. (Unless of course your INVOKE is different, which it had better not be) In Christ, Steven Watanabe
Steven Watanabe <watanabesj <at> gmail.com> writes:
AMDG
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late announce
The formal review of Paul Fultz II's Fit library starts today, 2nd March and ends on 13th March.
[...]
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so.
I've implemented the same thing in Hana, so I had to make the same design decision as Paul did at some point. I decided to be consistent with the rest of Hana, which is to capture by value. However, I am not sure that my decision in Hana was a wise one, and I am tempted to think that Paul's solution is actually the right one. Indeed, in most cases, `fit::capture` will be used as a temporary: some_alogrithm(..., fit::capture(variables...)(function)); In this case, capturing by reference is harmless. The only case where it can be harmful is if one "saves" the result of `fit::capture`, which could lead to (arguably ugly) dangling reference bugs: auto some_function() { std::string s = "foobar"; // oops, the return value contains a dangling reference to `s` return fit::capture(s)(f); } However, this should rarely happen. Of course, one could also suggest that instead of defaulting to capture-by-reference, the desire to capture by reference could be stated explicitly by using `std::ref` or some similar mechanism: fit::capture(std::ref(variables)...)(f) I'm not sure what's the best solution. Basically, my point is simply that saying "you should always capture by value" is naive in this context. Perhaps you're right, perhaps you're wrong, but in all cases this design choice is not an obvious one, at least not to me. Just my .02. Regards, Louis Dionne
AMDG On 03/03/2016 05:43 PM, Louis Dionne wrote:
Steven Watanabe <watanabesj <at> gmail.com> writes:
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote:
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so.
I've implemented the same thing in Hana, so I had to make the same design decision as Paul did at some point. I decided to be consistent with the rest of Hana, which is to capture by value.
However, I am not sure that my decision in Hana was a wise one, and I am tempted to think that Paul's solution is actually the right one. Indeed, in most cases, `fit::capture` will be used as a temporary:
some_alogrithm(..., fit::capture(variables...)(function));
In this case, capturing by reference is harmless. <snip> Basically, my point is simply that saying "you should always capture by value" is naive in this context. Perhaps you're right, perhaps you're wrong, but in all cases this design choice is not an obvious one, at least not to me. Just my .02.
You have a point, and perhaps capturing by reference makes sense. In fact, lambdas do capture by reference by default, so there is precedent for it. I do maintain, however, that choosing whether to capture by value based on l/r-valueness is definitely wrong. In Christ, Steven Watanabe
On Thursday, March 3, 2016 at 8:14:30 PM UTC-6, Steven Watanabe wrote:
AMDG
Steven Watanabe <watanabesj <at> gmail.com> writes:
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote:
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so.
I've implemented the same thing in Hana, so I had to make the same design decision as Paul did at some point. I decided to be consistent with the rest of Hana, which is to capture by value.
However, I am not sure that my decision in Hana was a wise one, and I am tempted to think that Paul's solution is actually the right one. Indeed, in most cases, `fit::capture` will be used as a temporary:
some_alogrithm(..., fit::capture(variables...)(function));
In this case, capturing by reference is harmless. <snip> Basically, my
On 03/03/2016 05:43 PM, Louis Dionne wrote: point is simply that
saying "you should always capture by value" is naive in this context. Perhaps you're right, perhaps you're wrong, but in all cases this design choice is not an obvious one, at least not to me. Just my .02.
You have a point, and perhaps capturing by reference makes sense. In fact, lambdas do capture by reference by default, so there is precedent for it. I do maintain, however, that choosing whether to capture by value based on l/r-valueness is definitely wrong.
Why is that wrong? Furthermore the name was chosen not for it to be default but based on the transformations it makes to the type categories. - capture_decay: Transform each category using decay - capture_forward: Transform each category to a reference. So that means rvalues become references. - capture: Dont transform the type category. Lvalues are captured as lvalues and rvalues are captured as rvalues. Furthermore, capturing rvalues by rvalue instead of reference helps improve safety, because rvalue references do not always extend the lifetime of a temporary. Paul
Le 04/03/2016 18:26, Paul Fultz II a écrit :
On Thursday, March 3, 2016 at 8:14:30 PM UTC-6, Steven Watanabe wrote:
AMDG
Steven Watanabe <watanabesj <at> gmail.com> writes:
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote: capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so. I've implemented the same thing in Hana, so I had to make the same design decision as Paul did at some point. I decided to be consistent with the rest of Hana, which is to capture by value.
However, I am not sure that my decision in Hana was a wise one, and I am tempted to think that Paul's solution is actually the right one. Indeed, in most cases, `fit::capture` will be used as a temporary:
some_alogrithm(..., fit::capture(variables...)(function));
In this case, capturing by reference is harmless. <snip> Basically, my
On 03/03/2016 05:43 PM, Louis Dionne wrote: point is simply that
saying "you should always capture by value" is naive in this context. Perhaps you're right, perhaps you're wrong, but in all cases this design choice is not an obvious one, at least not to me. Just my .02.
You have a point, and perhaps capturing by reference makes sense. In fact, lambdas do capture by reference by default, so there is precedent for it. I do maintain, however, that choosing whether to capture by value based on l/r-valueness is definitely wrong.
Why is that wrong? Furthermore the name was chosen not for it to be default but based on the transformations it makes to the type categories.
- capture_decay:
Transform each category using decay
- capture_forward:
Transform each category to a reference. So that means rvalues become references.
- capture:
Dont transform the type category. Lvalues are captured as lvalues and rvalues are captured as rvalues.
Furthermore, capturing rvalues by rvalue instead of reference helps improve safety, because rvalue references do not always extend the lifetime of a temporary.
What is the rational to capture all the same way? Why not have something like std::ref to change the way? Vicente
Le 04/03/2016 19:59, Vicente J. Botet Escriba a écrit :
Le 04/03/2016 18:26, Paul Fultz II a écrit :
On Thursday, March 3, 2016 at 8:14:30 PM UTC-6, Steven Watanabe wrote:
AMDG
Steven Watanabe <watanabesj <at> gmail.com> writes:
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote: capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so. I've implemented the same thing in Hana, so I had to make the same design decision as Paul did at some point. I decided to be consistent with the rest of Hana, which is to capture by value.
However, I am not sure that my decision in Hana was a wise one, and I am tempted to think that Paul's solution is actually the right one. Indeed, in most cases, `fit::capture` will be used as a temporary:
some_alogrithm(..., fit::capture(variables...)(function));
In this case, capturing by reference is harmless. <snip> Basically, my
On 03/03/2016 05:43 PM, Louis Dionne wrote: point is simply that
saying "you should always capture by value" is naive in this context. Perhaps you're right, perhaps you're wrong, but in all cases this design choice is not an obvious one, at least not to me. Just my .02.
You have a point, and perhaps capturing by reference makes sense. In fact, lambdas do capture by reference by default, so there is precedent for it. I do maintain, however, that choosing whether to capture by value based on l/r-valueness is definitely wrong.
Why is that wrong? Furthermore the name was chosen not for it to be default but based on the transformations it makes to the type categories.
- capture_decay: Transform each category using decay
- capture_forward: Transform each category to a reference. So that means rvalues become references.
- capture:
Dont transform the type category. Lvalues are captured as lvalues and rvalues are captured as rvalues.
Furthermore, capturing rvalues by rvalue instead of reference helps improve safety, because rvalue references do not always extend the lifetime of a temporary.
What is the rational to capture all the same way? Why not have something like std::ref to change the way?
If we consider that by default we capture by reference, can we capture a specific parameter by value? Vicente
On Friday, March 4, 2016 at 1:05:39 PM UTC-6, Vicente J. Botet Escriba wrote:
Le 04/03/2016 19:59, Vicente J. Botet Escriba a écrit :
Le 04/03/2016 18:26, Paul Fultz II a écrit :
On Thursday, March 3, 2016 at 8:14:30 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/03/2016 05:43 PM, Louis Dionne wrote:
Steven Watanabe <watanabesj <at> gmail.com> writes:
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote: capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so. I've implemented the same thing in Hana, so I had to make the same design decision as Paul did at some point. I decided to be consistent with the rest of Hana, which is to capture by value.
However, I am not sure that my decision in Hana was a wise one, and I am tempted to think that Paul's solution is actually the right one. Indeed, in most cases, `fit::capture` will be used as a temporary:
some_alogrithm(..., fit::capture(variables...)(function));
In this case, capturing by reference is harmless. <snip> Basically,
my
point is simply that
saying "you should always capture by value" is naive in this context. Perhaps you're right, perhaps you're wrong, but in all cases this design choice is not an obvious one, at least not to me. Just my .02.
You have a point, and perhaps capturing by reference makes sense. In fact, lambdas do capture by reference by default, so there is precedent for it. I do maintain, however, that choosing whether to capture by value based on l/r-valueness is definitely wrong.
Why is that wrong? Furthermore the name was chosen not for it to be default but based on the transformations it makes to the type categories.
- capture_decay: Transform each category using decay
- capture_forward: Transform each category to a reference. So that means rvalues become references.
- capture:
Dont transform the type category. Lvalues are captured as lvalues and rvalues are captured as rvalues.
Furthermore, capturing rvalues by rvalue instead of reference helps improve safety, because rvalue references do not always extend the lifetime of a temporary.
What is the rational to capture all the same way? Why not have something like std::ref to change the way?
If we consider that by default we capture by reference, can we capture a specific parameter by value?
Well, you would need to create some kind of value_wrapper(like reference_wrapper). Although, I would prefer not to have that much "magic" happen. If you want an lvalue, give it an lvalue, if you want an rvalue, give it an rvalue. Plus, the standard C++ language can be used to manipulate the type categories rather than relying on "magical" wrappers.
On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watanabesj@gmail.com> wrote:
AMDG
On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late announce
The formal review of Paul Fultz II's Fit library starts today, 2nd March and ends on 13th March.
We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost
No, this library should not be accepted into Boost at this time.
* Conditions for acceptance - Your name
Steven Watanabe
- Your knowledge of the problem domain.
High.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems?
msvc-12 (failed), msvc-14, clang-3.3 (failed), gcc-4.8. This is basically as expected.
- How much effort did you put into your evaluation of the review?
~5 hours. I only finished half a dozen files, but I've seen enough problems that I feel justified in stopping now.
Critical points:
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref.
- The library reinvents (poorly) its own versions of several components: compressed_pair <-> boost::compressed_pair,
As I recall boost::compressed_pair is never empty. In addition, it doesn't support constexpr. Plus, I structured it in a way to avoid bugs with gcc when doing EBO with constexpr.
placeholders <-> lambda, bind, phoenix
Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it handle perfect forwarding for more than 2 arguments.
Various configuration macros <-> Boost.Config
- The library attempts to take advantage of EBO, but makes several mistakes. (See attached tests which fail to compile)
Oops, this only affects compressed_pair(and is an easy fix), `pack` does not have this issue.
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable.
This is partly because of the fold done for older compiler. I could easily lift this restriction. Almost all of the adaptors use `ConstCallable`.
It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible.
Almost all of the function adaptor require that. Why is that weird?
Even worse are the functions (e.g. capture) which do not specify whether they require const or not.
That probably should be added.
You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly.
I probably should add Q/A for this. I make mutability explicit because for most cases `std::ref` should be used when using a mutable function object. If its really necessary(for example working with a third-party library) then `mutable_` adaptor can be used.
- The implementation has various useless bits, which serve no purpose that I can discern.
- Please be pedantic about ADL. If you aren't it /will/ bite you.
Good point.
Details:
Please add .gitattributes
test/Jamfile.v2: - project fit Project names must be globally unique, so it's better to use /boost/fit (or leave out the name entirely)
I didn't realize I could do that.
- [ test_all r ] "r" is completely meaningless here.
That is the way it was from where I copied it.
- Actually, everything from rule test-all down can be reduced to: for local fileb in [ glob *.cpp ] { run $(fileb) ; } You don't need to create a special target to group the tests, unless you want more granularity than "run everything" or "run specific tests by name."
Interesting. I really could find no reference on this information.
alias.hpp: 57: #pragma warning(disable: 4579) Library headers *must* disable warnings inside push/pop.
Noted.
67: std::false_type is defined somewhere... (It looks like you get <type_traits> via delegate.hpp) 74: Ditto for std::is_same
Yes, from delegate.hpp
93: BOOST_FIT_UNARY_PERFECT_ID macro is undocumented and unused.
Oops, yea it should be deleted.
107: I have no idea why you made alias_value variadic. The extra arguments are unused, their presence is undocumented, and all calls from inside the library (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) that use them, could just as well be written without.
This is to make `alias_value` depend on a template parameter because compiler eagerly instantiate template parameters.
117: struct alias_inherit #if (defined(__GNUC__) && !defined (__clang__)) : std::conditional<(std::is_class<T>::value), T, alias<T>>::type Really? alias_inherit is documented to use inheritence, so it should be an error to use it with a non-class type.
Yes it is an error to use something other than a class. This is to avoid a problem on gcc when calling `alias_value` that eagerly instantiates `alias_inherit`(even though I have an `enable_if` for `alias_value`).
145: // Since we disable the error for 4579 on MSVC, which leaves the static // member unitialized at runtime, it is, therefore, only safe to use this // class on types that are empty with constructors that have no possible // side effects. Shouldn't it also be safe for types with trivial default constructors, since then, zero-initialization will do the right thing?
That is probably another case as well.
always.hpp:
22: "The `always_ref` version will return a reference, and it requires the value passed in to be an lvalue." The first time I read this I thought it meant that the function object created by always_ref would be a reference, not that it produces a reference when called. Consider rephrasing the sentence to avoid the ambiguity. 39: "Requirements: T must be: * CopyConstructible" This only applies to always, not always_ref, no?
Yes.
61: #define BOOST_FIT_NO_CONSTEXPR_VOID 1 #ifndef BOOST_FIT_NO_CONSTEXPR_VOID ???
Oops, that is a mistake.
121: always_base has boost::fit::detail as an associated namespace.
Ah, I see, this could have the potential for strange ADL lookup. I'll probably need to move this stuff to their own namespace.
124: constexpr detail::always_base<void> operator()() const This overload is not documented. Should it be?
Yes, it is the case for always_void.
apply.hpp: 29: assert(compress(apply, f)(x, y, z) == f(x)(y)(z)); I don't think that compress should be part of the semantics of apply. This belongs as an example.
msvc-14.0 says: boost/fit/apply.hpp(83): warning C4003: not enough actual parameters for macro 'BOOST_FIT_APPLY_MEM_FN_CALL'
Emptiness is a correct argument. I probably should disable this warning.
apply_eval.hpp:
18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it.
That is the whole purpose of apply_eval, so the user can order the parameters that are applied to a function.
90: return eval_ordered<R>(f, pack_join(...), ...) Watch out for ADL.
I need a Steven Watanabe compiler.
113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested.
For what?
arg.hpp:
29: /// template<class IntegralConstant> /// constexpr auto arg(IntegralConstant); /// /// template<std::size_t N, class... Ts> /// constexpr auto arg_c(Ts&&...); arg and argc should do the same thing. It looks like arg creates a function object, while arg_c evaluates it directly.
Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the future, I may make this a template variable(so `arg_c<5>` will be a function object).
86: make_args_at(...)(..., make_perfect_ref(...)...) ADL...
95: get_args<N>(...) ADL
by.hpp:
8: BOOST_FIT_GUARD_FUNCTION_ON_H The #include guard doesn't match the file name
It originally was called `on`, thats why.
22: "Also, if just a projection is given, then the projection will be called for each of its arguments." I don't see any good reason for this to be an overload of by. It's essentially a for_each.
It seems like a natural extension: by(p, f)(xs...) == f(p(xs)...) by(p)(xs...) == p(xs)...;
25: Note: All projections are always evaluated in order from left-to-right. By only takes one projection.
Yea, that should read all the applications of the projection.
This is a lot of code to implement what is essentially a one-liner: apply_eval(f, capture_forward(x)(p)...)
It does that for older compilers, but on compilers that support it, it uses initializers lists.
135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow You should really have a single BOOST_FIT_VOID_RETURN and use it consistently everywhere.
The BOOST_FIT_BY_VOID_RETURN is only used in one place so it could be replaced with preprocessor ifs.
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so.
I don't see how it makes it more dangerous. Everything in scope is captured by reference and temporaries have there lifetimes extened so they will remain in scope as well. This works great when forwarding is added on.
79: return always_ref(*this)(xs...); Why not return *this? The always_ref dance serves no purpose, other than making the code hard to follow.
This is necessary because of the eagerness of constexpr.
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one.
Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
concepts.md:
Please reference the C++ standard when defining INVOKE. (Unless of course your INVOKE is different, which it had better not be)
Yes it is the same, I'll try to find the reference to the standard. Thanks for your feedback, Paul
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref.
`alway_ref should be just an implementation detail. The user can already write `always(std::ref(x)) if they want a reference. I only use this to try and save `some template instantiations when I need to the always_ref(*this)(xs...) `pattern.
145: // Since we disable the error for 4579 on MSVC, which leaves the static // member unitialized at runtime, it is, therefore, only safe to use this // class on types that are empty with constructors that have no possible // side effects. Shouldn't it also be safe for types with trivial default constructors, since then, zero-initialization will do the right thing?
That is probably another case as well.
Sorry this is wrong. The value is uninitialized. So its only valid when there is no data members to initialize(ie is_empty<T>) and the constructors have no side effects(ie is_literal<T>). Since it is uninitialized you can't expect it be zero-initialized either. Paul
Le 04/03/2016 18:37, paul Fultz a écrit :
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref. `alway_ref should be just an implementation detail. The user can already write `always(std::ref(x)) if they want a reference. I only use this to try and save `some template instantiations when I need to the always_ref(*this)(xs...) `pattern. I always_ref doesn't helps, could you create an issue to remove it?
Vicente
AMDG On 03/04/2016 10:37 AM, paul Fultz wrote:
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref.
`alway_ref should be just an implementation detail. The user can already write `always(std::ref(x)) if they want a reference. I only use this to try and save `some template instantiations when I need to the always_ref(*this)(xs...) `pattern.
If it's meant to be an implementation detail, then it shouldn't be documented. Also, how do you know that this optimization makes any difference? If it does make a difference, then wouldn't users want to take advantage of it too?
145: // Since we disable the error for 4579 on MSVC, which leaves the static // member unitialized at runtime, it is, therefore, only safe to use this // class on types that are empty with constructors that have no possible // side effects. Shouldn't it also be safe for types with trivial default constructors, since then, zero-initialization will do the right thing?
That is probably another case as well.
Sorry this is wrong. The value is uninitialized. So its only valid when there is no data members to initialize(ie is_empty<T>) and the constructors have no side effects(ie is_literal<T>). Since it is uninitialized you can't expect it be zero-initialized either.
Okay, I'm not familiar with this particular compiler bug. I was just making a wild guess based on [basic.start.init]: "Variables with static storage duration (3.7.1) or thread storage duration (3.7.2) shall be zero-initialized (8.5) before any other initialization takes place." In Christ, Steven Watanabe
On Friday, March 4, 2016 at 5:09:19 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/04/2016 10:37 AM, paul Fultz wrote:
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref.
`alway_ref should be just an implementation detail. The user can already write `always(std::ref(x)) if they want a reference. I only use this to try and save `some template instantiations when I need to the always_ref(*this)(xs...) `pattern.
If it's meant to be an implementation detail, then it shouldn't be documented. Also, how do you know that this optimization makes any difference? If it does make a difference, then wouldn't users want to take advantage of it too?
`always_ref` is used a lot throughout the library. For users of the library, it may not be used as often. Perhaps it would be better to leave it out for now, and then I can added it later, if there is need for this in other libraries or applications.
145: // Since we disable the error for 4579 on MSVC, which leaves the static // member unitialized at runtime, it is, therefore, only safe to use this // class on types that are empty with constructors that have no possible // side effects. Shouldn't it also be safe for types with trivial default constructors, since then, zero-initialization will do the right thing?
That is probably another case as well.
Sorry this is wrong. The value is uninitialized. So its only valid when there is no data members to initialize(ie is_empty<T>) and the constructors have no side effects(ie is_literal<T>). Since it is uninitialized you can't expect it be zero-initialized either.
Okay, I'm not familiar with this particular compiler bug. I was just making a wild guess based on [basic.start.init]: "Variables with static storage duration (3.7.1) or thread storage duration (3.7.2) shall be zero-initialized (8.5) before any other initialization takes place."
In Christ, Steven Watanabe
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 04/03/2016 03:16, paul Fultz a écrit :
On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watanabesj@gmail.com> wrote:
AMDG Thanks Steven for all these useful remarks. - How much effort did you put into your evaluation of the review?
~5 hours. I only finished half a dozen files, but I've seen enough problems that I feel justified in stopping now.
Critical points:
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref.
- The library reinvents (poorly) its own versions of several components: compressed_pair <-> boost::compressed_pair, As I recall boost::compressed_pair is never empty. In addition, it doesn't support constexpr. Plus, I structured it in a way to avoid bugs with gcc when doing EBO with constexpr. Could you provide a patch for boost::compressed_pair that is backward compatible and provides whatever you need? placeholders <-> lambda, bind, phoenix Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it handle perfect forwarding for more than 2 arguments. The same here.
Various configuration macros <-> Boost.Config
- The library attempts to take advantage of EBO, but makes several mistakes. (See attached tests which fail to compile) Oops, this only affects compressed_pair(and is an easy fix), `pack` does not have this issue. Please, could you create an issue and tag it review?
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable. This is partly because of the fold done for older compiler. I could easily lift this restriction. Almost all of the adaptors use `ConstCallable`. IIUC Steven, he is requesting to use Callable everywhere. Steven could you confirm?
It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible. Almost all of the function adaptor require that. Why is that weird? Why do you need that the Callable must be a ConstCallable?
Even worse are the functions (e.g. capture) which do not specify whether they require const or not. That probably should be added. Please, could you create an issue?
You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly. I probably should add Q/A for this. I make mutability explicit because for most cases `std::ref` should be used when using a mutable function object. If its really necessary(for example working with a third-party library) then `mutable_` adaptor can be used.
- The implementation has various useless bits, which serve no purpose that I can discern. Steven, could you be more explicit?
- Please be pedantic about ADL. If you aren't it /will/ bite you. Good point. Please, could you create an issue?
Details:
Please add .gitattributes Please, could you create an issue?
test/Jamfile.v2: Steven, Paul added the Jamfiles just before the review. There is not a lot of people that master BJam :( - project fit Project names must be globally unique, so it's better to use /boost/fit (or leave out the name entirely) I didn't realize I could do that. Please, could you create an issue?
- [ test_all r ] "r" is completely meaningless here. That is the way it was from where I copied it. From where you copied it?
- Actually, everything from rule test-all down can be reduced to: for local fileb in [ glob *.cpp ] { run $(fileb) ; } You don't need to create a special target to group the tests, unless you want more granularity than "run everything" or "run specific tests by name." Interesting. I really could find no reference on this information.
alias.hpp: 57: #pragma warning(disable: 4579) Library headers *must* disable warnings inside push/pop. Noted. Please, could you create an issue? 67: std::false_type is defined somewhere... (It looks like you get <type_traits> via delegate.hpp) 74: Ditto for std::is_same Yes, from delegate.hpp What Steven wants is that the dependencies be more explicit. If you use some traits you should include <type_traits>. Please, could you create an issue?
93: BOOST_FIT_UNARY_PERFECT_ID macro is undocumented and unused. Oops, yea it should be deleted. Please, could you create an issue?
107: I have no idea why you made alias_value variadic. The extra arguments are unused, their presence is undocumented, and all calls from inside the library (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) that use them, could just as well be written without. This is to make `alias_value` depend on a template parameter because compiler eagerly instantiate template parameters. Please could a clear explanation be added in the documentation? Please, could you create an issue?
117: struct alias_inherit #if (defined(__GNUC__) && !defined (__clang__)) : std::conditional<(std::is_class<T>::value), T, alias<T>>::type Really? alias_inherit is documented to use inheritence, so it should be an error to use it with a non-class type. Yes it is an error to use something other than a class. This is to avoid a problem on gcc when calling `alias_value` that eagerly instantiates `alias_inherit`(even though I have an `enable_if` for `alias_value`). Could this be documented in the code?
145: // Since we disable the error for 4579 on MSVC, which leaves the static // member unitialized at runtime, it is, therefore, only safe to use this // class on types that are empty with constructors that have no possible // side effects. Shouldn't it also be safe for types with trivial default constructors, since then, zero-initialization will do the right thing? That is probably another case as well. Please, could you create an issue?
always.hpp:
22: "The `always_ref` version will return a reference, and it requires the value passed in to be an lvalue." The first time I read this I thought it meant that the function object created by always_ref would be a reference, not that it produces a reference when called. Consider rephrasing the sentence to avoid the ambiguity. 39: "Requirements: T must be: * CopyConstructible" This only applies to always, not always_ref, no? Yes. Please, could you create an issue?
61: #define BOOST_FIT_NO_CONSTEXPR_VOID 1 #ifndef BOOST_FIT_NO_CONSTEXPR_VOID ??? Oops, that is a mistake. Please, could you create an issue?
121: always_base has boost::fit::detail as an associated namespace. Ah, I see, this could have the potential for strange ADL lookup. I'll probably need to move this stuff to their own namespace. Please, could you create an issue?
124: constexpr detail::always_base<void> operator()() const This overload is not documented. Should it be? Yes, it is the case for always_void. Please, could you create an issue?
apply.hpp: 29: assert(compress(apply, f)(x, y, z) == f(x)(y)(z)); I don't think that compress should be part of the semantics of apply. This belongs as an example.
msvc-14.0 says: boost/fit/apply.hpp(83): warning C4003: not enough actual parameters for macro 'BOOST_FIT_APPLY_MEM_FN_CALL' Emptiness is a correct argument. I probably should disable this warning. Please, could you create an issue?
apply_eval.hpp:
18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it. That is the whole purpose of apply_eval, so the user can order the parameters that are applied to a function. What about the test?
90: return eval_ordered<R>(f, pack_join(...), ...) Watch out for ADL. I need a Steven Watanabe compiler. Please, could you create an issue?
113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested. For what? What about the test?
arg.hpp:
29: /// template<class IntegralConstant> /// constexpr auto arg(IntegralConstant); /// /// template<std::size_t N, class... Ts> /// constexpr auto arg_c(Ts&&...); arg and argc should do the same thing. It looks like arg creates a function object, while arg_c evaluates it directly. Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the future, I may make this a template variable(so `arg_c<5>` will be a function object). What prevents you to doing it now?
86: make_args_at(...)(..., make_perfect_ref(...)...) ADL...
95: get_args<N>(...) ADL Associated those to the ADL issue. by.hpp:
8: BOOST_FIT_GUARD_FUNCTION_ON_H The #include guard doesn't match the file name It originally was called `on`, thats why. Please, could you create an issue?
22: "Also, if just a projection is given, then the projection will be called for each of its arguments." I don't see any good reason for this to be an overload of by. It's essentially a for_each. It seems like a natural extension:
by(p, f)(xs...) == f(p(xs)...) by(p)(xs...) == p(xs)...;
25: Note: All projections are always evaluated in order from left-to-right. By only takes one projection. Yea, that should read all the applications of the projection. Please, could you create an issue? This is a lot of code to implement what is essentially a one-liner: apply_eval(f, capture_forward(x)(p)...) It does that for older compilers, but on compilers that support it, it uses initializers lists. What are the old compilers? I don't know if it will be worth to have a branch for compilers conforming to C++14 and one for others.
135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow You should really have a single BOOST_FIT_VOID_RETURN and use it consistently everywhere. The BOOST_FIT_BY_VOID_RETURN is only used in one place so it could be replaced with preprocessor ifs. Please, could you create an issue?
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so. I don't see how it makes it more dangerous. Everything in scope is captured by reference and temporaries have there lifetimes extened so they will remain in scope as well. This works great when forwarding is added on.
79: return always_ref(*this)(xs...); Why not return *this? The always_ref dance serves no purpose, other than making the code hard to follow. This is necessary because of the eagerness of constexpr.
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one. Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
concepts.md:
Please reference the C++ standard when defining INVOKE. (Unless of course your INVOKE is different, which it had better not be) Yes it is the same, I'll try to find the reference to the standard.
Maybe you can reference http://en.cppreference.com/w/cpp/utility/functional/invoke There is already an issue for this point. Thanks again Steven. Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library? Best, Vicente
On Friday, March 4, 2016 at 12:56:05 PM UTC-6, Vicente J. Botet Escriba wrote:
Le 04/03/2016 03:16, paul Fultz a écrit :
On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watan...@gmail.com <javascript:>> wrote:
AMDG Thanks Steven for all these useful remarks. - How much effort did you put into your evaluation of the review?
~5 hours. I only finished half a dozen files, but I've seen enough problems that I feel justified in stopping now.
Critical points:
- Different functions in the library do not have a consistent interface for similar variations: - always/always_ref vs. capture/capture_forward/capture_decay vs partial w/ std::ref.
- The library reinvents (poorly) its own versions of several components: compressed_pair <-> boost::compressed_pair, As I recall boost::compressed_pair is never empty. In addition, it doesn't support constexpr. Plus, I structured it in a way to avoid bugs with gcc when doing EBO with constexpr. Could you provide a patch for boost::compressed_pair that is backward compatible and provides whatever you need?
Nope. Compressed pair is designed such that &x.first() != &x.second() is always the case. As some users may make assumptions about that.
placeholders <-> lambda, bind, phoenix Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it handle perfect forwarding for more than 2 arguments. The same here.
Making bind SFINAE-friendly breaks backwards compatibility. However, the lazy adapter passes the same tests for Boost.Bind, plus with constexpr.
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable. This is partly because of the fold done for older compiler. I could easily lift this restriction. Almost all of the adaptors use `ConstCallable`. IIUC Steven, he is requesting to use Callable everywhere. Steven could you confirm?
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
107: I have no idea why you made alias_value variadic. The extra arguments are unused, their presence is undocumented, and all calls from inside the library (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) that use them, could just as well be written without. This is to make `alias_value` depend on a template parameter because compiler eagerly instantiate template parameters. Please could a clear explanation be added in the documentation?
I don't know if it should be added to the documentation as its a implementation detail. Perhaps an extra comment could help.
arg.hpp:
29: /// template<class IntegralConstant> /// constexpr auto arg(IntegralConstant); /// /// template<std::size_t N, class... Ts> /// constexpr auto arg_c(Ts&&...); arg and argc should do the same thing. It looks like arg creates a function object, while arg_c evaluates it directly. Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In
the
future, I may make this a template variable(so `arg_c<5>` will be a function object). What prevents you to doing it now?
Time constraints mainly, but this applies to both arg_c and if_c.
86: make_args_at(...)(..., make_perfect_ref(...)...) ADL...
95: get_args<N>(...) ADL
Associated those to the ADL issue.
by.hpp:
8: BOOST_FIT_GUARD_FUNCTION_ON_H The #include guard doesn't match the file name It originally was called `on`, thats why. Please, could you create an issue?
22: "Also, if just a projection is given, then the projection will be called for each of its arguments." I don't see any good reason for this to be an overload of by. It's essentially a for_each. It seems like a natural extension:
by(p, f)(xs...) == f(p(xs)...) by(p)(xs...) == p(xs)...;
25: Note: All projections are always evaluated in order from left-to-right. By only takes one projection. Yea, that should read all the applications of the projection. Please, could you create an issue? This is a lot of code to implement what is essentially a one-liner: apply_eval(f, capture_forward(x)(p)...) It does that for older compilers, but on compilers that support it, it uses initializers lists. What are the old compilers? I don't know if it will be worth to have a branch for compilers conforming to C++14 and one for others.
Well, MSVC and gcc 4.8 is included in the older compilers, but I can make it work with Callable.
concepts.md:
Please reference the C++ standard when defining INVOKE. (Unless of course your INVOKE is different, which it had better not be) Yes it is the same, I'll try to find the reference to the standard.
Maybe you can reference http://en.cppreference.com/w/cpp/utility/functional/invoke
There is already an issue for this point.
Thanks again Steven.
Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library?
Yes this feedback was very useful, and would appreciate more feedback as well. I will open issues for the issues raised currently. More issues can be raised directly on the github page. Thanks, Paul
Le 04/03/2016 21:27, Paul Fultz II a écrit : Paul, snip the parts you don't replay to. And replay to the parts you acknowledge. If it you that create an issue, is a sign that you confirm already that the is an issue that must be addressed.
On Friday, March 4, 2016 at 12:56:05 PM UTC-6, Vicente J. Botet Escriba wrote:
Le 04/03/2016 03:16, paul Fultz a écrit : >> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watan...@gmail.com <javascript:>> wrote: >>> AMDG Thanks Steven for all these useful remarks. >>> - How much effort did you put into your evaluation of the review? >>> >> ~5 hours. I only finished half a dozen >> files, but I've seen enough problems that >> I feel justified in stopping now. >> >> Critical points: >> >> - Different functions in the library do not >> have a consistent interface for similar >> variations: >> - always/always_ref vs. capture/capture_forward/capture_decay vs >> partial w/ std::ref. >> >> - The library reinvents (poorly) its own versions >> of several components: >> compressed_pair <-> boost::compressed_pair, > As I recall boost::compressed_pair is never empty. In addition, it doesn't > support constexpr. Plus, I structured it in a way to avoid bugs with gcc when > doing EBO with constexpr. Could you provide a patch for boost::compressed_pair that is backward compatible and provides whatever you need?
Nope. Compressed pair is designed such that &x.first() != &x.second() is always the case. As some users may make assumptions about that.
Is this the single difference?
>> placeholders <-> lambda, bind, phoenix > Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it handle > perfect forwarding for more than 2 arguments. The same here.
Making bind SFINAE-friendly breaks backwards compatibility. However, the lazy adapter passes the same tests for Boost.Bind, plus with constexpr.
This should be documented as motivation of the addition of new placeholders.
> >> - Some library functions require function objects >> to have a const operator(), without any rhyme >> or reason. e.g. apply requires Callable, but >> apply_eval requires ConstCallable. > This is partly because of the fold done for older compiler. I could easily > lift this restriction. Almost all of the adaptors use `ConstCallable`. IIUC Steven, he is requesting to use Callable everywhere. Steven could you confirm?
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
We need to see the motivation for explicit mutable function objects. How your library work with standard function objects and other that user have written that conform to the standard requirements?
> >> 107: I have no idea why you made alias_value variadic. >> The extra arguments are unused, their presence is >> undocumented, and all calls from inside the library >> (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) >> that use them, could just as well be written without. > This is to make `alias_value` depend on a template parameter because compiler > eagerly instantiate template parameters. Please could a clear explanation be added in the documentation?
I don't know if it should be added to the documentation as its a implementation detail. Perhaps an extra comment could help.
Anything that justify an interface change should be described in the documentation.
> >> arg.hpp: >> >> 29: /// template<class IntegralConstant> >> /// constexpr auto arg(IntegralConstant); >> /// >> /// template<std::size_t N, class... Ts> >> /// constexpr auto arg_c(Ts&&...); >> arg and argc should do the same thing. It looks >> like arg creates a function object, while arg_c >> evaluates it directly. > Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the > future, I may make this a template variable(so `arg_c<5>` will be a function > object). What prevents you to doing it now?
Time constraints mainly, but this applies to both arg_c and if_c.
If this would change the interface it is better to do it before the library is accepted. If not could this be added to a future work section?
Thanks again Steven.
Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library?
Yes this feedback was very useful, and would appreciate more feedback as well. I will open issues for the issues raised currently. More issues can be raised directly on the github page.
I agree, but I would like to see them with a review label and I believe that only you can assign the labels. This will be very helpful to me. Vicente
On Saturday, March 5, 2016 at 3:02:18 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 04/03/2016 21:27, Paul Fultz II a écrit :
Paul, snip the parts you don't replay to. And replay to the parts you acknowledge. If it you that create an issue, is a sign that you confirm already that the is an issue that must be addressed.
I do confirm the issues pointed it out, I just didn't feel the need to repeat myself after everyline. Sorry.
On Friday, March 4, 2016 at 12:56:05 PM UTC-6, Vicente J. Botet Escriba wrote:
Le 04/03/2016 03:16, paul Fultz a écrit : >> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watan...@gmail.com <javascript:>> wrote: >>> AMDG Thanks Steven for all these useful remarks. >>> - How much effort did you put into your evaluation of the review? >>> >> ~5 hours. I only finished half a dozen >> files, but I've seen enough problems that >> I feel justified in stopping now. >> >> Critical points: >> >> - Different functions in the library do not >> have a consistent interface for similar >> variations: >> - always/always_ref vs. capture/capture_forward/capture_decay vs >> partial w/ std::ref. >> >> - The library reinvents (poorly) its own versions >> of several components: >> compressed_pair <-> boost::compressed_pair, > As I recall boost::compressed_pair is never empty. In addition, it doesn't > support constexpr. Plus, I structured it in a way to avoid bugs with gcc when > doing EBO with constexpr. Could you provide a patch for boost::compressed_pair that is backward compatible and provides whatever you need?
Nope. Compressed pair is designed such that &x.first() != &x.second() is always the case. As some users may make assumptions about that.
Is this the single difference?
>> placeholders <-> lambda, bind, phoenix > Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it handle > perfect forwarding for more than 2 arguments. The same here.
Making bind SFINAE-friendly breaks backwards compatibility. However, the lazy adapter passes the same tests for Boost.Bind, plus with constexpr.
This should be documented as motivation of the addition of new placeholders.
> >> - Some library functions require function objects >> to have a const operator(), without any rhyme >> or reason. e.g. apply requires Callable, but >> apply_eval requires ConstCallable. > This is partly because of the fold done for older compiler. I could easily > lift this restriction. Almost all of the adaptors use `ConstCallable`. IIUC Steven, he is requesting to use Callable everywhere. Steven could you confirm?
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
We need to see the motivation for explicit mutable function objects. How your library work with standard function objects and other that user have written that conform to the standard requirements?
> >> 107: I have no idea why you made alias_value variadic. >> The extra arguments are unused, their presence is >> undocumented, and all calls from inside the library >> (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) >> that use them, could just as well be written without. > This is to make `alias_value` depend on a template parameter because compiler > eagerly instantiate template parameters. Please could a clear explanation be added in the documentation?
I don't know if it should be added to the documentation as its a implementation detail. Perhaps an extra comment could help.
Anything that justify an interface change should be described in the documentation.
> >> arg.hpp: >> >> 29: /// template<class IntegralConstant> >> /// constexpr auto arg(IntegralConstant); >> /// >> /// template<std::size_t N, class... Ts> >> /// constexpr auto arg_c(Ts&&...); >> arg and argc should do the same thing. It looks >> like arg creates a function object, while arg_c >> evaluates it directly. > Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the > future, I may make this a template variable(so `arg_c<5>` will be a function > object). What prevents you to doing it now?
Time constraints mainly, but this applies to both arg_c and if_c.
If this would change the interface it is better to do it before the library is accepted. If not could this be added to a future work section?
Thanks again Steven.
Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library?
Yes this feedback was very useful, and would appreciate more feedback as well. I will open issues for the issues raised currently. More issues can be raised directly on the github page.
I agree, but I would like to see them with a review label and I believe that only you can assign the labels. This will be very helpful to me.
Alright, I will do that.
Vicente
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>> >> - The library reinvents (poorly) its own versions >> of several components: >> compressed_pair <-> boost::compressed_pair, > As I recall boost::compressed_pair is never empty. In addition, it doesn't > support constexpr. Plus, I structured it in a way to avoid bugs with gcc when > doing EBO with constexpr. Could you provide a patch for boost::compressed_pair that is backward compatible and provides whatever you need?
Nope. Compressed pair is designed such that &x.first() != &x.second() is always the case. As some users may make assumptions about that.
Is this the single difference?
That and it works with constexpr. Compressed pair is an implementation detail and is not part of the interface.
>> placeholders <-> lambda, bind, phoenix > Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it handle > perfect forwarding for more than 2 arguments. The same here.
Making bind SFINAE-friendly breaks backwards compatibility. However, the lazy adapter passes the same tests for Boost.Bind, plus with constexpr.
This should be documented as motivation of the addition of new placeholders.
I do mention that they support `constexpr`.
> >> - Some library functions require function objects >> to have a const operator(), without any rhyme >> or reason. e.g. apply requires Callable, but >> apply_eval requires ConstCallable. > This is partly because of the fold done for older compiler. I could easily > lift this restriction. Almost all of the adaptors use `ConstCallable`. IIUC Steven, he is requesting to use Callable everywhere. Steven could you confirm?
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
We need to see the motivation for explicit mutable function objects. How your library work with standard function objects and other that user have written that conform to the standard requirements?
Yes, I need to document this rationale.
> >> 107: I have no idea why you made alias_value variadic. >> The extra arguments are unused, their presence is >> undocumented, and all calls from inside the library >> (pack.hpp, combine.hpp, and detail/compressed_pair.hpp) >> that use them, could just as well be written without. > This is to make `alias_value` depend on a template parameter because compiler > eagerly instantiate template parameters. Please could a clear explanation be added in the documentation?
I don't know if it should be added to the documentation as its a implementation detail. Perhaps an extra comment could help.
Anything that justify an interface change should be described in the documentation.
Its not an interface change. Its just a workaround for gcc.
> >> arg.hpp: >> >> 29: /// template<class IntegralConstant> >> /// constexpr auto arg(IntegralConstant); >> /// >> /// template<std::size_t N, class... Ts> >> /// constexpr auto arg_c(Ts&&...); >> arg and argc should do the same thing. It looks >> like arg creates a function object, while arg_c >> evaluates it directly. > Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the > future, I may make this a template variable(so `arg_c<5>` will be a function > object). What prevents you to doing it now?
Time constraints mainly, but this applies to both arg_c and if_c.
If this would change the interface it is better to do it before the library is accepted. If not could this be added to a future work section?
This is not really an interface change. The change would be backwards compatible.
On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfultz2@yahoo.com> wrote:
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
If your library only requires Callable, then users can supply a Callable or a ConstCallable. When you require a ConstCallable, they cannot supply a Callable. There may be valid reasons for that restriction in certain cases, but they should be documented rather than just imposed as a convention. ___ Rob (Sent from my portable computation engine)
Rob Stewart wrote:
On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfultz2@yahoo.com> wrote:
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
I do not object to your requiring ConstCallable. I made boost/std::bind const-neutral because this felt the right thing to do - it's not bind's job to tell you what your function object ought to do, it just reflects whatever it does. At the same time, stateful function objects do have limited applicability outside of for_each which specifically guarantees order and returns the function object. You could pass bind( ++_1, 0 ) to generate_n and that's pretty much it. So I do not see the requirement as a defect warranting rejection. If users demand Callable support, you'll add it. If not, not.
If your library only requires Callable, then users can supply a Callable or a ConstCallable. When you require a ConstCallable, they cannot supply a Callable. There may be valid reasons for that restriction in certain cases, but they should be documented rather than just imposed as a convention.
That's true in principle. In practice though function objects that are not ConstCallable are in 90% of the cases the result of a forgotten 'const' on operator() and not a deliberate decision. So requiring ConstCallable catches errors.
Le 05/03/2016 13:22, Peter Dimov a écrit :
Rob Stewart wrote:
On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfultz2@yahoo.com> wrote:
The library could switch to using `Callable`, if everyone feels
that is > better. However, I honestly prefer to make mutable explicit. I would > like to hear feedback from other reviewers as well.
I do not object to your requiring ConstCallable. I made boost/std::bind const-neutral because this felt the right thing to do - it's not bind's job to tell you what your function object ought to do, it just reflects whatever it does. At the same time, stateful function objects do have limited applicability outside of for_each which specifically guarantees order and returns the function object. You could pass bind( ++_1, 0 ) to generate_n and that's pretty much it.
So I do not see the requirement as a defect warranting rejection. If users demand Callable support, you'll add it. If not, not.
If your library only requires Callable, then users can supply a Callable or a ConstCallable. When you require a ConstCallable, they cannot supply a Callable. There may be valid reasons for that restriction in certain cases, but they should be documented rather than just imposed as a convention.
That's true in principle. In practice though function objects that are not ConstCallable are in 90% of the cases the result of a forgotten 'const' on operator() and not a deliberate decision. So requiring ConstCallable catches errors.
For me the question is if the functions provided by the library need that the function object must be const or not. Vicente
On Saturday, March 5, 2016 at 6:40:49 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 13:22, Peter Dimov a écrit :
Rob Stewart wrote:
On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfu...@yahoo.com <javascript:>> wrote:
The library could switch to using `Callable`, if everyone feels
that is > better. However, I honestly prefer to make mutable explicit. I would > like to hear feedback from other reviewers as well.
I do not object to your requiring ConstCallable. I made boost/std::bind const-neutral because this felt the right thing to do - it's not bind's job to tell you what your function object ought to do, it just reflects whatever it does. At the same time, stateful function objects do have limited applicability outside of for_each which specifically guarantees order and returns the function object. You could pass bind( ++_1, 0 ) to generate_n and that's pretty much it.
So I do not see the requirement as a defect warranting rejection. If users demand Callable support, you'll add it. If not, not.
If your library only requires Callable, then users can supply a Callable or a ConstCallable. When you require a ConstCallable, they cannot supply a Callable. There may be valid reasons for that restriction in certain cases, but they should be documented rather than just imposed as a convention.
That's true in principle. In practice though function objects that are not ConstCallable are in 90% of the cases the result of a forgotten 'const' on operator() and not a deliberate decision. So requiring ConstCallable catches errors.
For me the question is if the functions provided by the library need that the function object must be const or not.
They do not need it. However, there can be some unexpected behavior when the user uses mutable function objects, because the functions can be copied or moved multiple times.
Vicente
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 05/03/2016 17:11, Paul Fultz II a écrit :
On Saturday, March 5, 2016 at 6:40:49 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 13:22, Peter Dimov a écrit : > Rob Stewart wrote: >> On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfu...@yahoo.com <javascript:>> >> wrote: >> > >> > The library could switch to using `Callable`, if everyone feels >> that is > better. However, I honestly prefer to make mutable >> explicit. I would > like to hear feedback from other reviewers as well. > > I do not object to your requiring ConstCallable. I made > boost/std::bind const-neutral because this felt the right thing to do > - it's not bind's job to tell you what your function object ought to > do, it just reflects whatever it does. At the same time, stateful > function objects do have limited applicability outside of for_each > which specifically guarantees order and returns the function object. > You could pass bind( ++_1, 0 ) to generate_n and that's pretty much it. > > So I do not see the requirement as a defect warranting rejection. If > users demand Callable support, you'll add it. If not, not. > >> If your library only requires Callable, then users can supply a >> Callable or a ConstCallable. When you require a ConstCallable, they >> cannot supply a Callable. There may be valid reasons for that >> restriction in certain cases, but they should be documented rather >> than just imposed as a convention. > > That's true in principle. In practice though function objects that are > not ConstCallable are in 90% of the cases the result of a forgotten > 'const' on operator() and not a deliberate decision. So requiring > ConstCallable catches errors. > For me the question is if the functions provided by the library need that the function object must be const or not.
They do not need it. However, there can be some unexpected behavior when the user uses mutable function objects, because the functions can be copied or moved multiple times.
Copied or moved? Does the library accepts move only function objects? Do you have an example of unexpected behavior? Vicente
On Saturday, March 5, 2016 at 4:43:45 PM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 17:11, Paul Fultz II a écrit :
On Saturday, March 5, 2016 at 6:40:49 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 13:22, Peter Dimov a écrit : > Rob Stewart wrote: >> On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfu...@yahoo.com <javascript:>> >> wrote: >> > >> > The library could switch to using `Callable`, if everyone feels >> that is > better. However, I honestly prefer to make mutable >> explicit. I would > like to hear feedback from other reviewers as well. > > I do not object to your requiring ConstCallable. I made > boost/std::bind const-neutral because this felt the right thing to do > - it's not bind's job to tell you what your function object ought to > do, it just reflects whatever it does. At the same time, stateful > function objects do have limited applicability outside of for_each > which specifically guarantees order and returns the function object. > You could pass bind( ++_1, 0 ) to generate_n and that's pretty much it. > > So I do not see the requirement as a defect warranting rejection. If > users demand Callable support, you'll add it. If not, not. > >> If your library only requires Callable, then users can supply a >> Callable or a ConstCallable. When you require a ConstCallable, they >> cannot supply a Callable. There may be valid reasons for that >> restriction in certain cases, but they should be documented rather >> than just imposed as a convention. > > That's true in principle. In practice though function objects that are > not ConstCallable are in 90% of the cases the result of a forgotten > 'const' on operator() and not a deliberate decision. So requiring > ConstCallable catches errors. > For me the question is if the functions provided by the library need that the function object must be const or not.
They do not need it. However, there can be some unexpected behavior when the user uses mutable function objects, because the functions can be copied
or
moved multiple times. Copied or moved?
I believe the issue is mainly with copying.
Does the library accepts move only function objects?
Yes it does.
Do you have an example of unexpected behavior?
This: struct counter { int i; counter() : i(0) {} template<class... Ts> int operator()(Ts&&...) { return i++; } }; counter c{}; by(mutable_(c))(1,1); // Prints 0 std::cout << c.i << std::endl; Instead `std::ref` should be used: counter c{}; by(std::ref(c))(1,1); // Prints 2 std::cout << c.i << std::endl;
On Saturday, March 5, 2016 at 6:22:53 AM UTC-6, Peter Dimov wrote:
Rob Stewart wrote:
On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfu...@yahoo.com <javascript:>> wrote:
The library could switch to using `Callable`, if everyone feels that
is
better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
I do not object to your requiring ConstCallable. I made boost/std::bind const-neutral because this felt the right thing to do - it's not bind's job to tell you what your function object ought to do, it just reflects whatever it does. At the same time, stateful function objects do have limited applicability outside of for_each which specifically guarantees order and returns the function object. You could pass bind( ++_1, 0 ) to generate_n and that's pretty much it.
So I do not see the requirement as a defect warranting rejection. If users demand Callable support, you'll add it. If not, not.
If your library only requires Callable, then users can supply a Callable or a ConstCallable. When you require a ConstCallable, they cannot supply a Callable. There may be valid reasons for that restriction in certain cases, but they should be documented rather than just imposed as a convention.
That's true in principle. In practice though function objects that are not ConstCallable are in 90% of the cases the result of a forgotten 'const' on operator() and not a deliberate decision. So requiring ConstCallable catches errors.
Furthermore, when I've needed a mutable function objects, many times I've needed to use `std::ref` to get the proper behavior.
On Saturday, March 5, 2016 at 5:15:01 AM UTC-6, Rob Stewart wrote:
On March 4, 2016 3:27:08 PM EST, Paul Fultz II <pfu...@yahoo.com <javascript:>> wrote:
The library could switch to using `Callable`, if everyone feels that is better. However, I honestly prefer to make mutable explicit. I would like to hear feedback from other reviewers as well.
If your library only requires Callable, then users can supply a Callable or a ConstCallable. When you require a ConstCallable, they cannot supply a Callable. There may be valid reasons for that restriction in certain cases, but they should be documented rather than just imposed as a convention.
Yes and if there are valid reasons for the mutable function object then the `mutable_` adaptor can be used. The mutable adaptor makes a mutable function object ConstCallable. Mutability is not prohibited, its just explicit.
___ Rob
(Sent from my portable computation engine)
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
AMDG On 03/04/2016 11:55 AM, Vicente J. Botet Escriba wrote:
Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library?
I suspect that I'd be saying a lot of the same things over again. For example: #include <boost/fit/conditional.hpp> struct identity { template<class T> T operator()(T t) { return t; } }; int main() { boost::fit::conditional(identity(), identity())(0); } 1> boost\fit\conditional.hpp(84): error C2500: 'boost::fit::detail::conditional_kernel<F1,F2>': 'identity' is already a direct base class 1> with 1> [ 1> F1=identity, 1> F2=identity 1> ] 1> In Christ, Steven Watanabe
On Friday, March 4, 2016 at 5:19:52 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/04/2016 11:55 AM, Vicente J. Botet Escriba wrote:
Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library?
I suspect that I'd be saying a lot of the same things over again. For example:
#include <boost/fit/conditional.hpp>
struct identity { template<class T> T operator()(T t) { return t; } };
int main() { boost::fit::conditional(identity(), identity())(0); }
1> boost\fit\conditional.hpp(84): error C2500: 'boost::fit::detail::conditional_kernel<F1,F2>': 'identity' is already a direct base class 1> with 1> [ 1> F1=identity, 1> F2=identity 1> ] 1>
This is by design. I should document this. It makes absolutely no sense to pass the same function to conditional, and is most likely a mistake in the users code.
In Christ, Steven Watanabe
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 05/03/2016 01:11, Paul Fultz II a écrit :
On Friday, March 4, 2016 at 5:19:52 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/04/2016 11:55 AM, Vicente J. Botet Escriba wrote:
Even if you are not for the library in its current state your comments will help a lot to improve it, so please, could you reconsider to give us more feedback on other parts of the library?
I suspect that I'd be saying a lot of the same things over again. For example:
#include <boost/fit/conditional.hpp>
struct identity { template<class T> T operator()(T t) { return t; } };
int main() { boost::fit::conditional(identity(), identity())(0); }
1> boost\fit\conditional.hpp(84): error C2500: 'boost::fit::detail::conditional_kernel<F1,F2>': 'identity' is already a direct base class 1> with 1> [ 1> F1=identity, 1> F2=identity 1> ] 1>
This is by design. I should document this. It makes absolutely no sense to pass the same function to conditional, and is most likely a mistake in the users code.
Please, could you add an issue on github to document the design? Vicente
AMDG On 03/05/2016 01:36 AM, Vicente J. Botet Escriba wrote:
Le 05/03/2016 01:11, Paul Fultz II a écrit :
On Friday, March 4, 2016 at 5:19:52 PM UTC-6, Steven Watanabe wrote:
On 03/04/2016 11:55 AM, Vicente J. Botet Escriba wrote:
#include <boost/fit/conditional.hpp>
struct identity { template<class T> T operator()(T t) { return t; } };
int main() { boost::fit::conditional(identity(), identity())(0); }
1> boost\fit\conditional.hpp(84): error C2500: 'boost::fit::detail::conditional_kernel<F1,F2>': 'identity' is already a direct base class 1> with 1> [ 1> F1=identity, 1> F2=identity 1> ] 1>
This is by design. I should document this. It makes absolutely no sense to pass the same function to conditional, and is most likely a mistake in the users code.
I disagree that this makes no sense. It would be true if we were talking about match, but for conditional, this degenerate case has a clear meaning, since conditional chooses the first possible function. Forbidding it creates a special case, and special cases are bad for generic code. Of course, my example is a bit silly, because I was trying to create a minimal reproduction, but it's quite possible to have a situation where you don't know and don't care whether the arguments are the same type. (warning: untested code) template<class S, class F> T my_accumulate(const S& s, T val, F f) { fusion::for_each(s, phoenix::ref(val) += phoenix::bind(conditional(f, identity), _1)); return val; } my_accumulate(fusion::make_vector(1, 2, 3), 0, identity); In Christ, Steven Watanabe
On Saturday, March 5, 2016 at 10:15:59 AM UTC-6, Steven Watanabe wrote:
AMDG
On 03/05/2016 01:36 AM, Vicente J. Botet Escriba wrote:
Le 05/03/2016 01:11, Paul Fultz II a écrit :
On Friday, March 4, 2016 at 5:19:52 PM UTC-6, Steven Watanabe wrote:
On 03/04/2016 11:55 AM, Vicente J. Botet Escriba wrote:
#include <boost/fit/conditional.hpp>
struct identity { template<class T> T operator()(T t) { return t; } };
int main() { boost::fit::conditional(identity(), identity())(0); }
1> boost\fit\conditional.hpp(84): error C2500: 'boost::fit::detail::conditional_kernel<F1,F2>': 'identity' is already
a
direct base class 1> with 1> [ 1> F1=identity, 1> F2=identity 1> ] 1>
This is by design. I should document this. It makes absolutely no sense to pass the same function to conditional, and is most likely a mistake in the users code.
I disagree that this makes no sense. It would be true if we were talking about match, but for conditional, this degenerate case has a clear meaning, since conditional chooses the first possible function. Forbidding it creates a special case, and special cases are bad for generic code. Of course, my example is a bit silly, because I was trying to create a minimal reproduction, but it's quite possible to have a situation where you don't know and don't care whether the arguments are the same type.
I see, so in generic code this could be a problem. I'll open an issue to update this.
Le 05/03/2016 17:36, Paul Fultz II a écrit :
On Saturday, March 5, 2016 at 10:15:59 AM UTC-6, Steven Watanabe wrote:
AMDG
Le 05/03/2016 01:11, Paul Fultz II a écrit :
On Friday, March 4, 2016 at 5:19:52 PM UTC-6, Steven Watanabe wrote:
On 03/04/2016 11:55 AM, Vicente J. Botet Escriba wrote: #include <boost/fit/conditional.hpp>
struct identity { template<class T> T operator()(T t) { return t; } };
int main() { boost::fit::conditional(identity(), identity())(0); }
1> boost\fit\conditional.hpp(84): error C2500: 'boost::fit::detail::conditional_kernel<F1,F2>': 'identity' is already a direct base class 1> with 1> [ 1> F1=identity, 1> F2=identity 1> ] 1>
This is by design. I should document this. It makes absolutely no sense to pass the same function to conditional, and is most likely a mistake in the users code.
I disagree that this makes no sense. It would be true if we were talking about match, but for conditional, this degenerate case has a clear meaning, since conditional chooses
On 03/05/2016 01:36 AM, Vicente J. Botet Escriba wrote: the first possible function. Forbidding it creates a special case, and special cases are bad for generic code. Of course, my example is a bit silly, because I was trying to create a minimal reproduction, but it's quite possible to have a situation where you don't know and don't care whether the arguments are the same type.
I see, so in generic code this could be a problem. I'll open an issue to update this.
AMDG On 03/03/2016 07:16 PM, paul Fultz wrote:
<snip>
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable.
This is partly because of the fold done for older compiler. I could easily lift this restriction. Almost all of the adaptors use `ConstCallable`.
It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible.
Almost all of the function adaptor require that. Why is that weird?
The fact that you require MoveConstructible implies that you're carrying around a copy. All the standard library algorithms take function objects by value, and do not constify them. std::not1/2 (and also the obsolete std::bind1st/2nd) do require const like you do, but std::bind does not, and std::function even has a const operator(), without requiring the same from its argument.
<snip> You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly.
I probably should add Q/A for this. I make mutability explicit because for most cases `std::ref` should be used when using a mutable function object. If its really necessary(for example working with a third-party library) then `mutable_` adaptor can be used.
You don't get to make up your own rules when you're dealing with long established concepts like function objects.
<snip>
Details:
<snip> - [ test_all r ] "r" is completely meaningless here.
That is the way it was from where I copied it.
Then your source is probably also wrong. All it does is to pass the string "r" as an argument to test_all, but since test_all doesn't use any arguments, it has no effect.
- Actually, everything from rule test-all down can be reduced to: for local fileb in [ glob *.cpp ] { run $(fileb) ; } You don't need to create a special target to group the tests, unless you want more granularity than "run everything" or "run specific tests by name."
Interesting. I really could find no reference on this information.
run etc. are documented here: http://www.boost.org/build/doc/html/bbv2/builtins/testing.html test-suite is at the bottom of: http://www.boost.org/build/doc/html/bbv2/reference/rules.html Once upon a time, I believe that test-suite was actually necessary. It has survived even to the present day, because most people create Jamfiles by copy-pasting existing Jamfiles.
<snip>
apply_eval.hpp:
18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it.
That is the whole purpose of apply_eval, so the user can order the parameters that are applied to a function.
Yes, but /why/? The name apply_eval doesn't convey any notion of evaluation order to me. It only matters for functions with side effects, and I wouldn't normally be using apply_eval on such functions in the first place. The examples that you provide don't depend on this, either.
<snip>
113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested.
For what?
In context: int x; template<class F, class... Ts> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) {} This constructor argument of x is essentially like: int x(1, 3); which is ill-formed. I'm presuming that you intended to use the comma operator, like so: x((apply(...), 0)) You didn't detect the problem because this template is never instantiated by your tests.
arg.hpp:
29: /// template<class IntegralConstant> /// constexpr auto arg(IntegralConstant); /// /// template<std::size_t N, class... Ts> /// constexpr auto arg_c(Ts&&...); arg and argc should do the same thing. It looks like arg creates a function object, while arg_c evaluates it directly.
Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the future, I may make this a template variable(so `arg_c<5>` will be a function object).
Making arg_c a variable template would answer my concerns.
<snip>
by.hpp:
22: "Also, if just a projection is given, then the projection will be called for each of its arguments." I don't see any good reason for this to be an overload of by. It's essentially a for_each.
It seems like a natural extension:
by(p, f)(xs...) == f(p(xs)...) by(p)(xs...) == p(xs)...;
Sure they're similar, but the name "by" makes no sense for the latter.
This is a lot of code to implement what is essentially a one-liner: apply_eval(f, capture_forward(x)(p)...)
It does that for older compilers, but on compilers that support it, it uses initializers lists.
My problem is that apply_eval already does all the necessary #ifdefing. Scattering #ifdefs around just makes the code unmaintainable.
135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow You should really have a single BOOST_FIT_VOID_RETURN and use it consistently everywhere.
The BOOST_FIT_BY_VOID_RETURN is only used in one place so it could be replaced with preprocessor ifs.
My point is that having multiple void_ types means that you have to keep track of which void_ you need depending on which other functions you use as implementation details.
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so.
I don't see how it makes it more dangerous. Everything in scope is captured by reference and temporaries have there lifetimes extened so they will remain in scope as well. This works great when forwarding is added on.
In general a function that accepts both lvalues and rvalues should have the same behavior for both, modulo optimizations. Anything that doesn't is just asking for trouble. In particular, I claim that if the function g returns a temporary, then the following transformation should either be valid and should leave the behavior unchanged, or it should fail to compile: f(g()); => auto x = g(); f(x);
79: return always_ref(*this)(xs...); Why not return *this? The always_ref dance serves no purpose, other than making the code hard to follow.
This is necessary because of the eagerness of constexpr.
I changed it and your tests still passed. I suspect that the problem that required this workaround was when you used BOOST_FIT_RETURNS or otherwise used this construct in the function prototype. There shouldn't be any problems when it only appears in the function body, as it does here.
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one.
Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
Exactly my point. In Christ, Steven Watanabe
On Friday, March 4, 2016 at 5:00:32 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/03/2016 07:16 PM, paul Fultz wrote:
<snip>
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable.
This is partly because of the fold done for older compiler. I could
easily
lift this restriction. Almost all of the adaptors use `ConstCallable`.
It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible.
Almost all of the function adaptor require that. Why is that weird?
The fact that you require MoveConstructible implies that you're carrying around a copy.
Yes that's correct.
All the standard library algorithms take function objects by value, and do not constify them.
Yep, and there's no guarantee how many times it gets copied in an algorithm. Which means using mutable objects could return different result depending how the algorithm decided to copy the function. This becomes even more apparent when people start using function adaptors as well. Because of these issues I decided to make mutability explicit. So the user will need to write `std::ref` or `boost::fit::mutable_` to use a mutable function.
std::not1/2 (and also the obsolete std::bind1st/2nd) do require const like you do, but std::bind does not, and std::function even has a const operator(), without requiring the same from its argument.
Yep, and std::bind suffers the same issue.
<snip> You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly.
I probably should add Q/A for this. I make mutability explicit because for most cases `std::ref` should be used when using a mutable function object. If its really necessary(for example working with a third-party library) then `mutable_` adaptor can be used.
You don't get to make up your own rules when you're dealing with long established concepts like function objects.
Whats so problematic with explicit mutability beyond that's not the way std::bind works?
<snip>
Details:
<snip> - [ test_all r ] "r" is completely meaningless here.
That is the way it was from where I copied it.
Then your source is probably also wrong. All it does is to pass the string "r" as an argument to test_all, but since test_all doesn't use any arguments, it has no effect.
- Actually, everything from rule test-all down can be reduced to: for local fileb in [ glob *.cpp ] { run $(fileb) ; } You don't need to create a special target to group the tests, unless you want more granularity than "run everything" or "run specific tests by name."
Interesting. I really could find no reference on this information.
run etc. are documented here: http://www.boost.org/build/doc/html/bbv2/builtins/testing.html test-suite is at the bottom of: http://www.boost.org/build/doc/html/bbv2/reference/rules.html
Once upon a time, I believe that test-suite was actually necessary. It has survived even to the present day, because most people create Jamfiles by copy-pasting existing Jamfiles.
<snip>
apply_eval.hpp:
18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it.
That is the whole purpose of apply_eval, so the user can order the parameters that are applied to a function.
Yes, but /why/? The name apply_eval doesn't convey any notion of evaluation order to me. It only matters for functions with side effects, and I wouldn't normally be using apply_eval on such functions in the first place. The examples that you provide don't depend on this, either.
I probably should find a better example.
<snip>
113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested.
For what?
In context: int x; template<class F, class... Ts> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) {}
This constructor argument of x is essentially like: int x(1, 3); which is ill-formed.
I'm presuming that you intended to use the comma operator, like so: x((apply(...), 0))
You didn't detect the problem because this template is never instantiated by your tests.
The tests don't run this because of `BOOST_FIT_NO_CONSTEXPR_VOID` is defined to 1, for all platforms. However, there are tests for this.
arg.hpp:
29: /// template<class IntegralConstant> /// constexpr auto arg(IntegralConstant); /// /// template<std::size_t N, class... Ts> /// constexpr auto arg_c(Ts&&...); arg and argc should do the same thing. It looks like arg creates a function object, while arg_c evaluates it directly.
Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In the future, I may make this a template variable(so `arg_c<5>` will be a function object).
Making arg_c a variable template would answer my concerns.
<snip>
by.hpp:
22: "Also, if just a projection is given, then the projection will be called for each of its arguments." I don't see any good reason for this to be an overload of by. It's essentially a for_each.
It seems like a natural extension:
by(p, f)(xs...) == f(p(xs)...) by(p)(xs...) == p(xs)...;
Sure they're similar, but the name "by" makes no sense for the latter.
This is a lot of code to implement what is essentially a one-liner: apply_eval(f, capture_forward(x)(p)...)
It does that for older compilers, but on compilers that support it, it uses initializers lists.
My problem is that apply_eval already does all the necessary #ifdefing. Scattering #ifdefs around just makes the code unmaintainable.
135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow You should really have a single BOOST_FIT_VOID_RETURN and use it consistently everywhere.
The BOOST_FIT_BY_VOID_RETURN is only used in one place so it could be replaced with preprocessor ifs.
My point is that having multiple void_ types means that you have to keep track of which void_ you need depending on which other functions you use as implementation details.
I can move to changing this to one void type.
capture.hpp:
32: // Capture lvalues by reference and rvalues by value. The default should be to capture everything by value. Capturing by reference is potentially dangerous, and switching based on whether the argument is an lvalue is even more so.
I don't see how it makes it more dangerous. Everything in scope is captured by reference and temporaries have there lifetimes extened so they will remain in scope as well. This works great when forwarding is added on.
In general a function that accepts both lvalues and rvalues should have the same behavior for both, modulo optimizations. Anything that doesn't is just asking for trouble.
In particular, I claim that if the function g returns a temporary, then the following transformation should either be valid and should leave the behavior unchanged, or it should fail to compile: f(g()); => auto x = g(); f(x);
I don't see how that prevents that.
79: return always_ref(*this)(xs...); Why not return *this? The always_ref dance serves no purpose, other than making the code hard to follow.
This is necessary because of the eagerness of constexpr.
I changed it and your tests still passed. I suspect that the problem that required this workaround was when you used BOOST_FIT_RETURNS or otherwise used this construct in the function prototype. There shouldn't be any problems when it only appears in the function body, as it does here.
There are probably some places where this can removed. I mostly like did it that way for consistency.
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one.
Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
Exactly my point.
In Christ, Steven Watanabe
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 05/03/2016 01:05, Paul Fultz II a écrit :
On Friday, March 4, 2016 at 5:00:32 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/03/2016 07:16 PM, paul Fultz wrote:
<snip>
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable. This is partly because of the fold done for older compiler. I could easily lift this restriction. Almost all of the adaptors use `ConstCallable`.
It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible. Almost all of the function adaptor require that. Why is that weird?
The fact that you require MoveConstructible implies that you're carrying around a copy.
Yes that's correct.
All the standard library algorithms take function objects by value, and do not constify them.
Yep, and there's no guarantee how many times it gets copied in an algorithm. Which means using mutable objects could return different result depending how the algorithm decided to copy the function. This becomes even more apparent when people start using function adaptors as well.
I would like to see an example where the problem is evident.
Because of these issues I decided to make mutability explicit. So the user will need to write `std::ref` or `boost::fit::mutable_` to use a mutable function.
It is a good practice to document the motivation of your design and show how the problem disappear. BTW, how fit::mutable_ is different from std::ref?
std::not1/2 (and also the obsolete std::bind1st/2nd) do require const like you do, but std::bind does not, and std::function even has a const operator(), without requiring the same from its argument.
Yep, and std::bind suffers the same issue.
<snip> You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly. I probably should add Q/A for this. I make mutability explicit because for most cases `std::ref` should be used when using a mutable function object. If its really necessary(for example working with a third-party library) then `mutable_` adaptor can be used.
You don't get to make up your own rules when you're dealing with long established concepts like function objects.
Whats so problematic with explicit mutability beyond that's not the way std::bind works?
There is no problem adding explicit mutability when it solves an problem.
<snip>
apply_eval.hpp:
18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it. That is the whole purpose of apply_eval, so the user can order the parameters that are applied to a function.
Yes, but /why/? The name apply_eval doesn't convey any notion of evaluation order to me. It only matters for functions with side effects, and I wouldn't normally be using apply_eval on such functions in the first place. The examples that you provide don't depend on this, either.
I probably should find a better example.
We will need this better example during the review to be able to say if it is useful.
<snip>
113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested. For what?
In context: int x; template<class F, class... Ts> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) {}
This constructor argument of x is essentially like: int x(1, 3); which is ill-formed.
I'm presuming that you intended to use the comma operator, like so: x((apply(...), 0))
You didn't detect the problem because this template is never instantiated by your tests.
The tests don't run this because of `BOOST_FIT_NO_CONSTEXPR_VOID` is defined to 1, for all platforms. However, there are tests for this.
Please, could you point to them? Could you add an issue to tract the configuration of a test with BOOST_FIT_NO_CONSTEXPR_VOID 0? Vicente
On Saturday, March 5, 2016 at 2:36:28 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 01:05, Paul Fultz II a écrit :
On Friday, March 4, 2016 at 5:00:32 PM UTC-6, Steven Watanabe wrote:
AMDG
On 03/03/2016 07:16 PM, paul Fultz wrote:
<snip>
- Some library functions require function objects to have a const operator(), without any rhyme or reason. e.g. apply requires Callable, but apply_eval requires ConstCallable. This is partly because of the fold done for older compiler. I could easily lift this restriction. Almost all of the adaptors use `ConstCallable`.
It's especially weird when a function (e.g. partial) requires both ConstCallable, and MoveConstructible. Almost all of the function adaptor require that. Why is that weird?
The fact that you require MoveConstructible implies that you're carrying around a copy.
Yes that's correct.
All the standard library algorithms take function objects by value, and do not constify them.
Yep, and there's no guarantee how many times it gets copied in an
algorithm.
Which means using mutable objects could return different result depending how the algorithm decided to copy the function. This becomes even more apparent when people start using function adaptors as well. I would like to see an example where the problem is evident. Because of these issues I decided to make mutability explicit. So the user will need to write `std::ref` or `boost::fit::mutable_` to use a mutable function.
It is a good practice to document the motivation of your design and show how the problem disappear. BTW, how fit::mutable_ is different from std::ref?
std::ref just stores a pointer to the object, whereas fit::mutable_ stores a copy of the object.
std::not1/2 (and also the obsolete std::bind1st/2nd) do require const like you do, but std::bind does not, and std::function even has a const operator(), without requiring the same from its argument.
Yep, and std::bind suffers the same issue.
<snip> You should not be requiring const at all, for anything. See [func.bind.bind] for an interface that handles const correctly. I probably should add Q/A for this. I make mutability explicit because for most cases `std::ref` should be used when using a mutable function object. If its really necessary(for example working with a third-party library) then `mutable_` adaptor can be used.
You don't get to make up your own rules when you're dealing with long established concepts like function objects.
Whats so problematic with explicit mutability beyond that's not the way std::bind works?
There is no problem adding explicit mutability when it solves an problem.
<snip>
apply_eval.hpp:
18: "Each [`eval`](eval.md) call is always ordered from left-to-right." It isn't clear to me why this guarantee is important. Not to mention that the test case apply_eval.cpp doesn't check it. That is the whole purpose of apply_eval, so the user can order the parameters that are applied to a function.
Yes, but /why/? The name apply_eval doesn't convey any notion of evaluation order to me. It only matters for functions with side effects, and I wouldn't normally be using apply_eval on such functions in the first place. The examples that you provide don't depend on this, either.
I probably should find a better example.
We will need this better example during the review to be able to say if it is useful.
<snip>
113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) You need parens around the comma operator. Also not tested. For what?
In context: int x; template<class F, class... Ts> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) {}
This constructor argument of x is essentially like: int x(1, 3); which is ill-formed.
I'm presuming that you intended to use the comma operator, like so: x((apply(...), 0))
You didn't detect the problem because this template is never instantiated by your tests.
The tests don't run this because of `BOOST_FIT_NO_CONSTEXPR_VOID` is
defined
to 1, for all platforms. However, there are tests for this.
Please, could you point to them? Could you add an issue to tract the configuration of a test with BOOST_FIT_NO_CONSTEXPR_VOID 0?
There is a line that needs to be removed in the source code, which defines it to 1, this disables the configuration of the macro based on the platform. I didn't realized that this line was in the source code. Essentially, what happens is that it falls back to using an alternative for `void` even when its supported on newer platforms. There is a test for `apply_eval` at line 15, that checks it being called with a function returning `void`. So, essentially, the code pointed out by steven is never used by the library. However, once I remove the define, the tests will fail, and I will need to correct the problem.
Vicente
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 05/03/2016 16:48, Paul Fultz II a écrit :
On Saturday, March 5, 2016 at 2:36:28 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 01:05, Paul Fultz II a écrit : > > On Friday, March 4, 2016 at 5:00:32 PM UTC-6, Steven Watanabe wrote:
std::ref just stores a pointer to the object, whereas fit::mutable_ stores a copy of the object.
I see.
>> std::not1/2 (and also the obsolete std::bind1st/2nd) >> do require const like you do, but std::bind does not, >> and std::function even has a const operator(), >> without requiring the same from its argument. >> > Yep, and std::bind suffers the same issue. > > >>>> <snip> >>>> You should not be requiring >>>> const at all, for anything. See [func.bind.bind] >>>> for an interface that handles const correctly. >>> I probably should add Q/A for this. I make mutability explicit because >> for >>> most cases `std::ref` should be used when using a mutable function >> object. If >>> its really necessary(for example working with a third-party library) >> then >>> `mutable_` adaptor can be used. >>> >> You don't get to make up your own rules >> when you're dealing with long established >> concepts like function objects. >> > Whats so problematic with explicit mutability beyond that's not the way > std::bind works? > There is no problem adding explicit mutability when it solves an problem. >>>> <snip> >>>> >>>> apply_eval.hpp: >>>> >>>> 18: "Each [`eval`](eval.md <http://eval.md>) call is always ordered >>>> from left-to-right." >>>> It isn't clear to me why this guarantee is important. >>>> Not to mention that the test case apply_eval.cpp >>>> doesn't check it. >>> That is the whole purpose of apply_eval, so the user can order the >> parameters >>> that are applied to a function. >>> >> Yes, but /why/? The name apply_eval >> doesn't convey any notion of evaluation >> order to me. It only matters for functions >> with side effects, and I wouldn't normally >> be using apply_eval on such functions in >> the first place. The examples that you >> provide don't depend on this, either. >> > I probably should find a better example. > We will need this better example during the review to be able to say if it is useful. > >>>> <snip> >>>> >>>> 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) >>>> You need parens around the comma operator. >>>> Also not tested. >>> For what? >>> >> In context: >> int x; >> template<class F, class... Ts> >> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, >> BOOST_FIT_FORWARD(Ts)(xs)...), 0) >> {} >> >> This constructor argument of x is essentially like: >> int x(1, 3); >> which is ill-formed. >> >> I'm presuming that you intended to use the >> comma operator, like so: >> x((apply(...), 0)) >> >> You didn't detect the problem because >> this template is never instantiated by >> your tests. >> > > The tests don't run this because of `BOOST_FIT_NO_CONSTEXPR_VOID` is defined > to 1, for all platforms. However, there are tests for this. > Please, could you point to them? Could you add an issue to tract the configuration of a test with BOOST_FIT_NO_CONSTEXPR_VOID 0?
There is a line that needs to be removed in the source code, which defines it to 1, this disables the configuration of the macro based on the platform. I didn't realized that this line was in the source code. Essentially, what happens is that it falls back to using an alternative for `void` even when its supported on newer platforms. There is a test for `apply_eval` at line 15, that checks it being called with a function returning `void`.
So, essentially, the code pointed out by steven is never used by the library. However, once I remove the define, the tests will fail, and I will need to correct the problem.
Please, could you add an issue, if not already done? Vicente
On Saturday, March 5, 2016 at 5:07:58 PM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 16:48, Paul Fultz II a écrit :
On Saturday, March 5, 2016 at 2:36:28 AM UTC-6, Vicente J. Botet Escriba wrote:
Le 05/03/2016 01:05, Paul Fultz II a écrit : > > On Friday, March 4, 2016 at 5:00:32 PM UTC-6, Steven Watanabe wrote:
std::ref just stores a pointer to the object, whereas fit::mutable_ stores a copy of the object.
I see.
>> std::not1/2 (and also the obsolete std::bind1st/2nd) >> do require const like you do, but std::bind does not, >> and std::function even has a const operator(), >> without requiring the same from its argument. >> > Yep, and std::bind suffers the same issue. > > >>>> <snip> >>>> You should not be requiring >>>> const at all, for anything. See [func.bind.bind] >>>> for an interface that handles const correctly. >>> I probably should add Q/A for this. I make mutability explicit because >> for >>> most cases `std::ref` should be used when using a mutable function >> object. If >>> its really necessary(for example working with a third-party library) >> then >>> `mutable_` adaptor can be used. >>> >> You don't get to make up your own rules >> when you're dealing with long established >> concepts like function objects. >> > Whats so problematic with explicit mutability beyond that's not the way > std::bind works? > There is no problem adding explicit mutability when it solves an problem. >>>> <snip> >>>> >>>> apply_eval.hpp: >>>> >>>> 18: "Each [`eval`](eval.md <http://eval.md>) call is always ordered >>>> from left-to-right." >>>> It isn't clear to me why this guarantee is important. >>>> Not to mention that the test case apply_eval.cpp >>>> doesn't check it. >>> That is the whole purpose of apply_eval, so the user can order the >> parameters >>> that are applied to a function. >>> >> Yes, but /why/? The name apply_eval >> doesn't convey any notion of evaluation >> order to me. It only matters for functions >> with side effects, and I wouldn't normally >> be using apply_eval on such functions in >> the first place. The examples that you >> provide don't depend on this, either. >> > I probably should find a better example. > We will need this better example during the review to be able to say if it is useful. > >>>> <snip> >>>> >>>> 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0) >>>> You need parens around the comma operator. >>>> Also not tested. >>> For what? >>> >> In context: >> int x; >> template<class F, class... Ts> >> constexpr eval_helper(const F& f, Ts&&... xs) : x(apply(f, >> BOOST_FIT_FORWARD(Ts)(xs)...), 0) >> {} >> >> This constructor argument of x is essentially like: >> int x(1, 3); >> which is ill-formed. >> >> I'm presuming that you intended to use the >> comma operator, like so: >> x((apply(...), 0)) >> >> You didn't detect the problem because >> this template is never instantiated by >> your tests. >> > > The tests don't run this because of `BOOST_FIT_NO_CONSTEXPR_VOID` is defined > to 1, for all platforms. However, there are tests for this. > Please, could you point to them? Could you add an issue to tract the configuration of a test with BOOST_FIT_NO_CONSTEXPR_VOID 0?
There is a line that needs to be removed in the source code, which defines it to 1, this disables the configuration of the macro based on the platform. I didn't realized that this line was in the source code. Essentially, what happens is that it falls back to using an alternative for `void` even when its supported on newer platforms. There is a test for `apply_eval` at line
15,
that checks it being called with a function returning `void`.
So, essentially, the code pointed out by steven is never used by the library. However, once I remove the define, the tests will fail, and I will need to correct the problem. Please, could you add an issue, if not already done?
That is part of this issue here: https://github.com/pfultz2/Fit/issues/113
Vicente
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Steven Watanabe wrote:
On 03/03/2016 07:16 PM, paul Fultz wrote:
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one.
Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
Exactly my point.
I actually like this another lambda library, and I'm not sure why it's half-baked. The reason I like it is that when I proposed std::bind I deliberately put into it the hooks (is_placeholder, is_bind_expression) that allow a simple lambda library to be written to extend it, so that one can write f.ex. bind( f, _1 ) + bind( f, _2 ) * 2. And with C++11, writing this extension is almost trivial. I've been trying to find the time to do so, and write an article about it, but no luck so far. Well, Paul wrote one. We've also been struggling with the theoretically sound way to define _1 in a header, and Paul provides a solution for that as well. (The one suggestion I have regarding this portion of the library is that Fit's placeholders should specialize boost::is_placeholder so that they could be used with boost::bind.)
On Saturday, March 5, 2016 at 4:20:31 AM UTC-6, Peter Dimov wrote:
Steven Watanabe wrote:
On 03/03/2016 07:16 PM, paul Fultz wrote:
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one.
Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
Exactly my point.
I actually like this another lambda library, and I'm not sure why it's half-baked.
The reason I like it is that when I proposed std::bind I deliberately put into it the hooks (is_placeholder, is_bind_expression) that allow a simple lambda library to be written to extend it, so that one can write f.ex. bind( f, _1 ) + bind( f, _2 ) * 2. And with C++11, writing this extension is almost trivial. I've been trying to find the time to do so, and write an article about it, but no luck so far. Well, Paul wrote one.
We've also been struggling with the theoretically sound way to define _1 in a header, and Paul provides a solution for that as well.
(The one suggestion I have regarding this portion of the library is that Fit's placeholders should specialize boost::is_placeholder so that they could be used with boost::bind.)
That is good point, and I will add an issue for that.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 05/03/2016 17:00, Paul Fultz II a écrit :
On Saturday, March 5, 2016 at 4:20:31 AM UTC-6, Peter Dimov wrote:
Steven Watanabe wrote:
On 03/03/2016 07:16 PM, paul Fultz wrote:
placeholders.hpp:
We really don't need another lambda library, let alone a half-baked one. Its not designed to be a full lambda library. Just enough to handle some constexpr cases.
Exactly my point. I actually like this another lambda library, and I'm not sure why it's half-baked.
The reason I like it is that when I proposed std::bind I deliberately put into it the hooks (is_placeholder, is_bind_expression) that allow a simple lambda library to be written to extend it, so that one can write f.ex. bind( f, _1 ) + bind( f, _2 ) * 2. And with C++11, writing this extension is almost trivial. I've been trying to find the time to do so, and write an article about it, but no luck so far. Well, Paul wrote one.
We've also been struggling with the theoretically sound way to define _1 in a header, and Paul provides a solution for that as well.
(The one suggestion I have regarding this portion of the library is that Fit's placeholders should specialize boost::is_placeholder so that they could be used with boost::bind.)
That is good point, and I will add an issue for that.
Hi, just a recall, the review of the Fit library started 2nd March and ends on 13th March. Paul has written a lot of articles related to his libraries in his blog http://pfultz2.com/blog/. I would like to see a lot of things from his blogs in the library documentation and/or have references to them. What do you think? Vicente Le 03/03/2016 12:43, Vicente J. Botet Escriba a écrit :
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.
Fit is:
- Modern: Fit takes advantages of modern C++11/C++14 features. It support both `constexpr` initialization and `constexpr` evaluation of functions. It takes advantage of type deduction, varidiac templates, and perfect forwarding to provide a simple and modern interface.
- Relevant: Fit provides utilities for functions and does not try to implement a functional language in C++. As such, Fit solves many problems relevant to C++ programmers, including initialization of function objects and lambdas, overloading with ordering, improved return type deduction, and much more.
- Lightweight: Fit builds simple lightweight abstraction on top of function objects. It does not require subscribing to an entire framework. Just use the parts you need.
Fit is divided into three components:
* Function Adaptors and Decorators: These enhance functions with additional capability.
* Functions: These return functions that achieve a specific purpose.
* Utilities: These are general utilities that are useful when defining or using functions
Fit has been tested on gcc 4.6-4.9, clang 3.4-3.7, and Visual Studio 2015.
For more information see:
Github:https://github.com/pfultz2/Fit/tree/boost
Documentation:http://pfultz2.github.io/Fit/doc/html/
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 - Your name - Your knowledge of the problem domain.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems? - How much effort did you put into your evaluation of the review?
More information about the review process can be found here:http://www.boost.org/community/reviews.html
We await your feedback!
Best regards, Vicente J. Botet Escriba
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Hi, 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. Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?) John M., Peter D. what do you think? Steven, were you thinking on other parts of the library? Paul, it is normal that you added those utilities in your library, as Boost doesn't provides them, however them can be included as sub-modules of other libraries. Best, Vicente returns doc http://pfultz2.github.io/Fit/doc/html/returns/index.html file https://github.com/pfultz2/Fit/blob/boost/include/boost/fit/returns.hpp function doc http://pfultz2.github.io/Fit/doc/html/function/index.html file https://github.com/pfultz2/Fit/blob/boost/include/boost/fit/function.hpp lambda doc http://pfultz2.github.io/Fit/doc/html/lambda/index.html file https://github.com/pfultz2/Fit/blob/boost/include/boost/fit/lambda.hpp lift http://pfultz2.github.io/Fit/doc/html/lift/index.html file https://github.com/pfultz2/Fit/blob/boost/include/boost/fit/lift.hpp
On 2016-03-06 05:21, Vicente J. Botet Escriba wrote:
Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?)
I don't think Boost.Core is the right place for them as this library is for small generally useful components used by many libraries. The components you pointed out seem too specialized to me, and Boost.Fit looks like the right place for them.
Le 06/03/2016 09:59, Andrey Semashev a écrit :
On 2016-03-06 05:21, Vicente J. Botet Escriba wrote:
Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?)
I don't think Boost.Core is the right place for them as this library is for small generally useful components used by many libraries. The components you pointed out seem too specialized to me, and Boost.Fit looks like the right place for them.
|Andrey, I was thinking in Boost.Core as these are language-like emulation features, like e.g. addressof, enable_if, explicit_operator_bool, ignorunused, no_exception_support, noncopyanble, scoped_enum, typeinfo. IIUC, BOOST_FIT_STATIC_FUNCTION try to fix a standard Core issue (pending issue 2104 in CWG) identified by Eric Nibler, with the a solution based on proposal's Eric (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4381.html). The library has also a macro BOOST_FIT_DECLARE_STATIC_VAR to do it for any data variable. I would like something that solves this problem in Boost. BOOST_FIT_STATIC_LAMBDA try to covers the C++17 feature constexpr lambdas. I suspect that Boost.Hana should have something like that. Louis, could you tell us how do you manage with these issues? IMO, The fiw of those issues is independent from a functional utilities library, even if Fit can not work without them.|| This is way I believe that it should be extracted and Boost.Core seems the good place to me. Best, Vicente |
On 2016-03-06 12:59, Vicente J. Botet Escriba wrote:
Le 06/03/2016 09:59, Andrey Semashev a écrit :
On 2016-03-06 05:21, Vicente J. Botet Escriba wrote:
Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?)
I don't think Boost.Core is the right place for them as this library is for small generally useful components used by many libraries. The components you pointed out seem too specialized to me, and Boost.Fit looks like the right place for them.
|Andrey, I was thinking in Boost.Core as these are language-like emulation features, like e.g. addressof, enable_if, explicit_operator_bool, ignorunused, no_exception_support, noncopyanble, scoped_enum, typeinfo.
IIUC, BOOST_FIT_STATIC_FUNCTION try to fix a standard Core issue (pending issue 2104 in CWG) identified by Eric Nibler, with the a solution based on proposal's Eric (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4381.html). The library has also a macro BOOST_FIT_DECLARE_STATIC_VAR to do it for any data variable. I would like something that solves this problem in Boost.
I don't mind having that in Boost (in fact, I welcome it) but these tools are specific to functional domain, AFAIU. I think Boost.Phoenix has something similar as well. Maybe, if there's a common ground in these tools, they should all be unified and extracted to a new separate library.
On Mar 6, 2016, at 04:59, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:
Le 06/03/2016 09:59, Andrey Semashev a écrit :
On 2016-03-06 05:21, Vicente J. Botet Escriba wrote:
Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?)
I don't think Boost.Core is the right place for them as this library is for small generally useful components used by many libraries. The components you pointed out seem too specialized to me, and Boost.Fit looks like the right place for them.
Andrey, I was thinking in Boost.Core as these are language-like emulation features, like e.g. addressof, enable_if, explicit_operator_bool, ignorunused, no_exception_support, noncopyanble, scoped_enum, typeinfo.
IIUC, BOOST_FIT_STATIC_FUNCTION try to fix a standard Core issue (pending issue 2104 in CWG) identified by Eric Nibler, with the a solution based on proposal's Eric (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4381.html). The library has also a macro BOOST_FIT_DECLARE_STATIC_VAR to do it for any data variable. I would like something that solves this problem in Boost.
BOOST_FIT_STATIC_LAMBDA try to covers the C++17 feature constexpr lambdas.
I suspect that Boost.Hana should have something like that. Louis, could you tell us how do you manage with these issues?
I don’t manage these issues. Hana uses only hand-written function objects that can be marked constexpr, and lambdas are completely excluded from the codebase. There’s no way to properly workaround the limitations of constexpr lambdas, since the lambda’s operator() won’t be constexpr even with BOOST_FIT_STATIC_LAMBDA. Regards, Louis
Le 06/03/2016 15:29, Louis Dionne a écrit :
On Mar 6, 2016, at 04:59, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:
Le 06/03/2016 09:59, Andrey Semashev a écrit :
On 2016-03-06 05:21, Vicente J. Botet Escriba wrote:
Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?) I don't think Boost.Core is the right place for them as this library is for small generally useful components used by many libraries. The components you pointed out seem too specialized to me, and Boost.Fit looks like the right place for them.
Andrey, I was thinking in Boost.Core as these are language-like emulation features, like e.g. addressof, enable_if, explicit_operator_bool, ignorunused, no_exception_support, noncopyanble, scoped_enum, typeinfo.
IIUC, BOOST_FIT_STATIC_FUNCTION try to fix a standard Core issue (pending issue 2104 in CWG) identified by Eric Nibler, with the a solution based on proposal's Eric (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4381.html). The library has also a macro BOOST_FIT_DECLARE_STATIC_VAR to do it for any data variable. I would like something that solves this problem in Boost.
BOOST_FIT_STATIC_LAMBDA try to covers the C++17 feature constexpr lambdas.
I suspect that Boost.Hana should have something like that. Louis, could you tell us how do you manage with these issues? I don’t manage these issues. Hana uses only hand-written function objects that can be marked constexpr, and lambdas are completely excluded from the codebase. There’s no way to properly workaround the limitations of constexpr lambdas, since the lambda’s operator() won’t be constexpr even with BOOST_FIT_STATIC_LAMBDA.
Sorry it was me that did the association of BOOST_FIT_STATIC_LAMBDA to C++17 constexpr lambdas.
"The |BOOST_FIT_STATIC_LAMBDA| macro allows initializing non-capturing lambdas at compile-time in a |constexpr| expression."
Nevertheless we get the ability to declare them at the namespace level, which is already a good step, or do you consider that this is not useful? Paul, if Louis is right, I believe that the documentation should state clearly that even if the resulting function can be assigned to a constexpr variable the user can not call it at compile-time, and even if this is obvious for the experts. Vicente
On Sunday, March 6, 2016 at 8:30:19 AM UTC-6, Louis Dionne wrote:
On Mar 6, 2016, at 04:59, Vicente J. Botet Escriba <vicent...@wanadoo.fr <javascript:>> wrote:
On 2016-03-06 05:21, Vicente J. Botet Escriba wrote:
Others could be considered also as function.hpp, lambda.hpp and lift.hpp, as the macros are there to workaround some missing language features, but those are much more specialized (Boost.Core?)
I don't think Boost.Core is the right place for them as this library is for small generally useful components used by many libraries. The components you pointed out seem too specialized to me, and Boost.Fit looks
Le 06/03/2016 09:59, Andrey Semashev a écrit : like the right place for them.
Andrey, I was thinking in Boost.Core as these are language-like emulation features, like e.g. addressof, enable_if, explicit_operator_bool, ignorunused, no_exception_support, noncopyanble, scoped_enum, typeinfo.
IIUC, BOOST_FIT_STATIC_FUNCTION try to fix a standard Core issue (pending issue 2104 in CWG) identified by Eric Nibler, with the a solution based on proposal's Eric ( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4381.html). The library has also a macro BOOST_FIT_DECLARE_STATIC_VAR to do it for any data variable. I would like something that solves this problem in Boost.
BOOST_FIT_STATIC_LAMBDA try to covers the C++17 feature constexpr lambdas.
I suspect that Boost.Hana should have something like that. Louis, could you tell us how do you manage with these issues?
I don’t manage these issues. Hana uses only hand-written function objects that can be marked constexpr, and lambdas are completely excluded from the codebase. There’s no way to properly workaround the limitations of constexpr lambdas, since the lambda’s operator() won’t be constexpr even with BOOST_FIT_STATIC_LAMBDA.
That is correct. It only allows for constexpr initialization of the lambda, which is why I call it `STATIC_LAMBDA` instead of `CONSTEXPR_LAMBDA`. I probably should add some more documentation to make it clearer. Also note, that the Fit library only uses functions objects as well. The only place that lambdas are used is for the lift operator.
AMDG Another (obscure) reason to be careful about using inheritance: #include <boost/fit/flip.hpp> struct base { base(int) {} }; struct derived : virtual base { derived() : base(1) {} void operator()(int, void *) const {} }; int main() { boost::fit::flip(derived())(nullptr, 2); } boost/fit/flip.hpp(56): error C2512: 'base::base': no appropriate default constructor available In Christ, Steven Watanabe
On 3/3/2016 6:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late anounce
The formal review of Paul Fultz II's Fit library starts today, 2nd March and ends on 13th March.
Fit is a header-only C++11/C++14 library that provides utilities for functions and function objects.
These are some comments/queries about the Fit documentation. Introduction The introduction says that Fit "provides utilities for functions and function objects." But it seems as if Fit works only with lambda functions and function objects. The term 'function' normally encompasses a much larger definition in C++ which includes global functions, static functions, member functions, and lambda functions. Fit needs to be more precise in what it says it works with. It repeatedly refers to function objects and lambda functions as 'functions'. I think this vagueness of terminality is really confusing in the documentation. Quick Start:Function Objects "We can make it behave like a regular function if we construct the class as a global variable." What about a non-global sum_f sum = sum_f(); makes 'sum' not behave like a regular function other than the fact that the variable 'sum' may eventually go out of scope ? Quick Start:Lambdas Why do we need both BOOST_FIT_STATIC_LAMBDA and BOOST_FIT_STATIC_LAMBDA_FUNCTION ? I would seem that BOOST_FIT_STATIC_LAMBDA_FUNCTION would be adequate and BOOST_FIT_STATIC_LAMBDA is just redundant, mimicking lambda syntax to no purpose. Quick Start:Overloading The overloading adaptors show two or more lambda functions. Can they also work with function objects ? Or a mix of lambda functions and function objects ? In fact all the reamining Quick Start topics show examples with lambda functions. Do they also work with function objects ? Quick Start:Variadic I do not understand what 'We can also make this print function varidiac, so it prints every argument passed into it.' means ? I do not think the Quick Start explains very much since it is dealing with adaptors of which we know almost nothing and the explanation for these adaptors and what they actually do is very terse. I realize that is why it is called a 'Quick Start' is because it is just giving the end-user a quick review of the sort of functionality that the library entails, but I would much rather see a good Overview first before I look at anything else in the documentation. More Examples The more examples section looks at useful cases for the library. Since I haven't had an overview for the library yet it is disappointing that this section comes before I really understand how the library is organized with at least general functionality. I am now given a series of some highly complicated library syntax without the least idea of why any of this syntax should be as it is, what it means, or how it works. That doesn't convince me to use the library at all. It just convinces me that the library has some showy functionality which does clever things for particular use cases which the library author wants to convince me I am going to encounter in my own programming. Overview The overview section consists of explaining what a function adaptor, static function adaptor, and decorator are. It also give me some notes about the syntax of the documentation. I would call this section 'Definitions' and "Documentation Syntax'. One thing it is not is an overview of the Fit library as far as I can understand what an overview is. I also note that following large scale sections deal in Adaptors, Decorators, Functions, and Utilities. Since 'Functions' and 'Utilities' are not "defined" in this Overview I am left to wonder if 'Functions' and 'Utilities' just mean anything in the library which is not an adaptor or a decorator, and what distinguishes a function from a utility. In the 'Signatures' section of the Overview I read: "All the functions are global function objects except where an explicit template parameter is required." I honestly don't know what this is supposed to mean. Does this refer to when function objects are referred to as parameters to the adaptors, functions, and utilities of the library ? The rest of the general About section is fairly straightforward. I still have no idea of the general functional of the library and why I should use it. I will only gain an idea of what the library does by reading the specifics of each adaptor, decorator, function, and utility. This seems to be the gist of the documentation to me. If I want to understand what the library does for me in working with function objects and lambda functions I have to read the specific documentation of each entity in the library and then go back to the Quick Start and More Examples sections to further understand how some of these entities are used. I think the library documentation would have been much better if there were a discussion of what the adaptors, decorators, functions, and utilities of the library were meant to do to enhance user programming rather than give examples here and there of usage and then leave it up to the end-user to figure out what, why, and how these entities are to be used. Quite frankly I don't have a use for a library that does not attempt to present itself as offering functionality that would be useful for me in the domain in which the library operates. if I have little or no idea of that domain I see no reason to think of using a library. Since Boost has a number of other libraries dealing with function objects, most notable the Boost Phoenix library, which is a sort of successor to the Boost Lambda library, I would think it would be advantage to the library author to stress functionality in the Fit library which goes beyond, improves on, or is different from the functionality in Boost Phoenix, in order to 'sell' end-users on the use of the Fit library in their code. I do realize that the Fit library deals with C++11/C++14 idioms, which means that it may be difficult for many end-users to understand, but I don't think it presents its documentation in such a way that makes its functionality easier to understand and I think it should do so. This does not mean that I will vote right now for Fit to be accepted or not accepted as a Boost library. I have to look much more carefully at the individual functionality of the adaptors, decorators, functions, and utilities of the library to understand what it actually does, before I can vote one way or another. But I hope my comments and questions about the documentation to the library will aid the library author in improving the documentation for end users like me, who are very interested in library enhancements to function object/lambda function functionality, but who find the documentation difficult and limiting in certain ways.
On Sunday, March 6, 2016 at 9:42:19 PM UTC-6, Edward Diener wrote:
On 3/3/2016 6:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late anounce
The formal review of Paul Fultz II's Fit library starts today, 2nd March and ends on 13th March.
Fit is a header-only C++11/C++14 library that provides utilities for functions and function objects.
These are some comments/queries about the Fit documentation.
Introduction
The introduction says that Fit "provides utilities for functions and function objects." But it seems as if Fit works only with lambda functions and function objects.
Fit works with any generalized Callable: http://en.cppreference.com/w/cpp/concept/Callable
The term 'function' normally encompasses a much larger definition in C++ which includes global functions, static functions, member functions, and lambda functions.
Which is what Callable encompasses, but I used "function" since more people are familiar with that than Callable.
Fit needs to be more precise in what it says it works with. It repeatedly refers to function objects and lambda functions as 'functions'. I think this vagueness of terminality is really confusing in the documentation.
Perhaps, I should refer to the Callable concept early on then.
Quick Start:Function Objects
"We can make it behave like a regular function if we construct the class as a global variable."
What about a non-global
sum_f sum = sum_f();
makes 'sum' not behave like a regular function other than the fact that the variable 'sum' may eventually go out of scope ?
In C++, a regular function is always global, there is no such thing as local function(sans gcc extensions).
Quick Start:Lambdas
Why do we need both BOOST_FIT_STATIC_LAMBDA and BOOST_FIT_STATIC_LAMBDA_FUNCTION ? I would seem that BOOST_FIT_STATIC_LAMBDA_FUNCTION would be adequate and BOOST_FIT_STATIC_LAMBDA is just redundant, mimicking lambda syntax to no purpose.
BOOST_FIT_STATIC_LAMBDA_FUNCTION and BOOST_FIT_STATIC_FUNCTION both define a function at global scope, and can only be used at global scope, whereas BOOST_FIT_STATIC_LAMBDA can be used to constexpr initialize local variables as well. In fact, BOOST_FIT_LIFT uses this since it is not always clear what context the user might call BOOST_FIT_LIFT.
Quick Start:Overloading
The overloading adaptors show two or more lambda functions. Can they also work with function objects ? Or a mix of lambda functions and function objects ? In fact all the reamining Quick Start topics show examples with lambda functions. Do they also work with function objects ?
Yes it can be used with function objects. I probably should show an example of that as well. I used the lambdas because of the terseness of them.
Quick Start:Variadic
I do not understand what 'We can also make this print function varidiac, so it prints every argument passed into it.' means ?
I'll try to explain that better, but basically it will print each argument, so: print("hello", 5); // Will print "hello" and 5
I do not think the Quick Start explains very much since it is dealing with adaptors of which we know almost nothing and the explanation for these adaptors and what they actually do is very terse.
Probably can expand the explanation of adaptors a little more.
I realize that is why it is called a 'Quick Start' is because it is just giving the end-user a quick review of the sort of functionality that the library entails, but I would much rather see a good Overview first before I look at anything else in the documentation.
More Examples
The more examples section looks at useful cases for the library. Since I haven't had an overview for the library yet it is disappointing that this section comes before I really understand how the library is organized with at least general functionality. I am now given a series of some highly complicated library syntax without the least idea of why any of this syntax should be as it is, what it means, or how it works. That doesn't convince me to use the library at all. It just convinces me that the library has some showy functionality which does clever things for particular use cases which the library author wants to convince me I am going to encounter in my own programming.
Probably more explanation about each of the components used to build the example will be helpful.
Overview
The overview section consists of explaining what a function adaptor, static function adaptor, and decorator are. It also give me some notes about the syntax of the documentation. I would call this section 'Definitions' and "Documentation Syntax'. One thing it is not is an overview of the Fit library as far as I can understand what an overview is. I also note that following large scale sections deal in Adaptors, Decorators, Functions, and Utilities. Since 'Functions' and 'Utilities' are not "defined" in this Overview I am left to wonder if 'Functions' and 'Utilities' just mean anything in the library which is not an adaptor or a decorator, and what distinguishes a function from a utility.
This is a good point, the current overview should be called "Definitions".
In the 'Signatures' section of the Overview I read:
"All the functions are global function objects except where an explicit template parameter is required." I honestly don't know what this is supposed to mean. Does this refer to when function objects are referred to as parameters to the adaptors, functions, and utilities of the library ?
I don't understand what you are asking. It means that the function is written like this in the documentation: template<class IntegralConstant> constexpr auto if_(IntegralConstant); But its actually a function object like this in the code: struct if_f { template<class IntegralConstant> constexpr auto operator()(IntegralConstant) const; }; const constexpr if_f if_ = {}; However, `if_c` is written like this in the documentation: template<bool B, class F> constexpr auto if_c(F); It requires the bool `B` template parameter explicity. So in the code it is written as a function and not as a function object.
The rest of the general About section is fairly straightforward. I still have no idea of the general functional of the library and why I should use it. I will only gain an idea of what the library does by reading the specifics of each adaptor, decorator, function, and utility. This seems to be the gist of the documentation to me. If I want to understand what the library does for me in working with function objects and lambda functions I have to read the specific documentation of each entity in the library and then go back to the Quick Start and More Examples sections to further understand how some of these entities are used.
I think the library documentation would have been much better if there were a discussion of what the adaptors, decorators, functions, and utilities of the library were meant to do to enhance user programming rather than give examples here and there of usage and then leave it up to the end-user to figure out what, why, and how these entities are to be used. Quite frankly I don't have a use for a library that does not attempt to present itself as offering functionality that would be useful for me in the domain in which the library operates. if I have little or no idea of that domain I see no reason to think of using a library.
Since Boost has a number of other libraries dealing with function objects, most notable the Boost Phoenix library, which is a sort of successor to the Boost Lambda library, I would think it would be advantage to the library author to stress functionality in the Fit library which goes beyond, improves on, or is different from the functionality in Boost Phoenix, in order to 'sell' end-users on the use of the Fit library in their code. I do realize that the Fit library deals with C++11/C++14 idioms, which means that it may be difficult for many end-users to understand, but I don't think it presents its documentation in such a way that makes its functionality easier to understand and I think it should do so. This does not mean that I will vote right now for Fit to be accepted or not accepted as a Boost library. I have to look much more carefully at the individual functionality of the adaptors, decorators, functions, and utilities of the library to understand what it actually does, before I can vote one way or another. But I hope my comments and questions about the documentation to the library will aid the library author in improving the documentation for end users like me, who are very interested in library enhancements to function object/lambda function functionality, but who find the documentation difficult and limiting in certain ways.
Thanks for your feedback, and it will be very helpful in improving the documentation.
On 3/7/2016 12:15 AM, Paul Fultz II wrote:
On Sunday, March 6, 2016 at 9:42:19 PM UTC-6, Edward Diener wrote:
On 3/3/2016 6:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late anounce
The formal review of Paul Fultz II's Fit library starts today, 2nd March and ends on 13th March.
Fit is a header-only C++11/C++14 library that provides utilities for functions and function objects.
These are some comments/queries about the Fit documentation.
Introduction
The introduction says that Fit "provides utilities for functions and function objects." But it seems as if Fit works only with lambda functions and function objects.
Fit works with any generalized Callable:
Please document this. A generalized Callable is a much bigger set of functionality than just lambda functions and function objects.
The term 'function' normally encompasses a much larger definition in C++ which includes global functions, static functions, member functions, and lambda functions.
Which is what Callable encompasses, but I used "function" since more people are familiar with that than Callable.
A 'function' and a Callable in C++ are two entirely different things. Use the one that is precise and don't worry what people are familiar with. Explain what a Callable is early on ( I know you explain it later ) if you feel that people do not know what it means.
Fit needs to be more precise in what it says it works with. It repeatedly refers to function objects and lambda functions as 'functions'. I think this vagueness of terminality is really confusing in the documentation.
Perhaps, I should refer to the Callable concept early on then.
See above.
Quick Start:Function Objects
"We can make it behave like a regular function if we construct the class as a global variable."
What about a non-global
sum_f sum = sum_f();
makes 'sum' not behave like a regular function other than the fact that the variable 'sum' may eventually go out of scope ?
In C++, a regular function is always global, there is no such thing as local function(sans gcc extensions).
My point is that the non-global 'sum' in my example above behaves just as much like a regular function as your global 'sum'. You may want to promote the idea of global function objects but I think that this is personally a bad idea. IMO global variables of any kind are to be avoided.
Quick Start:Lambdas
Why do we need both BOOST_FIT_STATIC_LAMBDA and BOOST_FIT_STATIC_LAMBDA_FUNCTION ? I would seem that BOOST_FIT_STATIC_LAMBDA_FUNCTION would be adequate and BOOST_FIT_STATIC_LAMBDA is just redundant, mimicking lambda syntax to no purpose.
BOOST_FIT_STATIC_LAMBDA_FUNCTION and BOOST_FIT_STATIC_FUNCTION both define a function at global scope, and can only be used at global scope, whereas BOOST_FIT_STATIC_LAMBDA can be used to constexpr initialize local variables as well.
It might be good to add that when the end-user first encounters BOOST_FIT_STATIC_LAMBDA, because the name does not suggest initialization of local objects. Furthermore I cannot imagine why one would want to use it to initialize a local object, so you might want to explain the benefit of doing so as opposed to the normal syntax for creating a lambda function.
In fact, BOOST_FIT_LIFT uses this since it is not always clear what context the user might call BOOST_FIT_LIFT.
Quick Start:Overloading
The overloading adaptors show two or more lambda functions. Can they also work with function objects ? Or a mix of lambda functions and function objects ? In fact all the reamining Quick Start topics show examples with lambda functions. Do they also work with function objects ?
Yes it can be used with function objects. I probably should show an example of that as well. I used the lambdas because of the terseness of them.
Evidently the adaptors can be used with any Callable. I think you should make that point very strongly and show that in examples also.
Quick Start:Variadic
I do not understand what 'We can also make this print function varidiac, so it prints every argument passed into it.' means ?
I'll try to explain that better, but basically it will print each argument, so:
print("hello", 5); // Will print "hello" and 5
I wouldn't associate that with the word 'Variadic' but I do think you need to explain that more clearly.
I do not think the Quick Start explains very much since it is dealing with adaptors of which we know almost nothing and the explanation for these adaptors and what they actually do is very terse.
Probably can expand the explanation of adaptors a little more.
Good idea. What do adaptors create ? Different function objects I would imagine ? In that case how about the explanation that adaptors take Callables as input and generate function object types that adapt the original functionality of one or more Callables to some other purpose.
I realize that is why it is called a 'Quick Start' is because it is just giving the end-user a quick review of the sort of functionality that the library entails, but I would much rather see a good Overview first before I look at anything else in the documentation.
More Examples
The more examples section looks at useful cases for the library. Since I haven't had an overview for the library yet it is disappointing that this section comes before I really understand how the library is organized with at least general functionality. I am now given a series of some highly complicated library syntax without the least idea of why any of this syntax should be as it is, what it means, or how it works. That doesn't convince me to use the library at all. It just convinces me that the library has some showy functionality which does clever things for particular use cases which the library author wants to convince me I am going to encounter in my own programming.
Probably more explanation about each of the components used to build the example will be helpful.
Overview
The overview section consists of explaining what a function adaptor, static function adaptor, and decorator are. It also give me some notes about the syntax of the documentation. I would call this section 'Definitions' and "Documentation Syntax'. One thing it is not is an overview of the Fit library as far as I can understand what an overview is. I also note that following large scale sections deal in Adaptors, Decorators, Functions, and Utilities. Since 'Functions' and 'Utilities' are not "defined" in this Overview I am left to wonder if 'Functions' and 'Utilities' just mean anything in the library which is not an adaptor or a decorator, and what distinguishes a function from a utility.
This is a good point, the current overview should be called "Definitions".
I would suggest an Overview that discusses the main functionality of adaptors, decorators, functions, and utilities.
In the 'Signatures' section of the Overview I read:
"All the functions are global function objects except where an explicit template parameter is required." I honestly don't know what this is supposed to mean. Does this refer to when function objects are referred to as parameters to the adaptors, functions, and utilities of the library ?
I don't understand what you are asking. It means that the function is written like this in the documentation:
template<class IntegralConstant> constexpr auto if_(IntegralConstant);
But its actually a function object like this in the code:
struct if_f { template<class IntegralConstant> constexpr auto operator()(IntegralConstant) const; }; const constexpr if_f if_ = {};
However, `if_c` is written like this in the documentation:
template<bool B, class F> constexpr auto if_c(F);
It requires the bool `B` template parameter explicity. So in the code it is written as a function and not as a function object.
I don't understand to what you are referring when you say 'function'. Are you talking about adaptors in your library, functions in your library, or what ? Please try to understand that your use of the word 'function' is very broad but that the word 'function' in C++ has a much narrower meaning.
The rest of the general About section is fairly straightforward. I still have no idea of the general functional of the library and why I should use it. I will only gain an idea of what the library does by reading the specifics of each adaptor, decorator, function, and utility. This seems to be the gist of the documentation to me. If I want to understand what the library does for me in working with function objects and lambda functions I have to read the specific documentation of each entity in the library and then go back to the Quick Start and More Examples sections to further understand how some of these entities are used.
I think the library documentation would have been much better if there were a discussion of what the adaptors, decorators, functions, and utilities of the library were meant to do to enhance user programming rather than give examples here and there of usage and then leave it up to the end-user to figure out what, why, and how these entities are to be used. Quite frankly I don't have a use for a library that does not attempt to present itself as offering functionality that would be useful for me in the domain in which the library operates. if I have little or no idea of that domain I see no reason to think of using a library.
Since Boost has a number of other libraries dealing with function objects, most notable the Boost Phoenix library, which is a sort of successor to the Boost Lambda library, I would think it would be advantage to the library author to stress functionality in the Fit library which goes beyond, improves on, or is different from the functionality in Boost Phoenix, in order to 'sell' end-users on the use of the Fit library in their code. I do realize that the Fit library deals with C++11/C++14 idioms, which means that it may be difficult for many end-users to understand, but I don't think it presents its documentation in such a way that makes its functionality easier to understand and I think it should do so. This does not mean that I will vote right now for Fit to be accepted or not accepted as a Boost library. I have to look much more carefully at the individual functionality of the adaptors, decorators, functions, and utilities of the library to understand what it actually does, before I can vote one way or another. But I hope my comments and questions about the documentation to the library will aid the library author in improving the documentation for end users like me, who are very interested in library enhancements to function object/lambda function functionality, but who find the documentation difficult and limiting in certain ways.
Thanks for your feedback, and it will be very helpful in improving the documentation.
On Monday, March 7, 2016 at 2:52:09 AM UTC-6, Edward Diener wrote:
On 3/7/2016 12:15 AM, Paul Fultz II wrote:
On Sunday, March 6, 2016 at 9:42:19 PM UTC-6, Edward Diener wrote:
On 3/3/2016 6:43 AM, Vicente J. Botet Escriba wrote:
Dear Boost community, sorry for the late anounce
The formal review of Paul Fultz II's Fit library starts today, 2nd
March
and ends on 13th March.
Fit is a header-only C++11/C++14 library that provides utilities for functions and function objects.
These are some comments/queries about the Fit documentation.
Introduction
The introduction says that Fit "provides utilities for functions and function objects." But it seems as if Fit works only with lambda functions and function objects.
Fit works with any generalized Callable:
Please document this. A generalized Callable is a much bigger set of functionality than just lambda functions and function objects.
The term 'function' normally encompasses a much larger definition in C++ which includes global functions, static functions, member functions, and lambda functions.
Which is what Callable encompasses, but I used "function" since more people are familiar with that than Callable.
A 'function' and a Callable in C++ are two entirely different things. Use the one that is precise and don't worry what people are familiar with. Explain what a Callable is early on ( I know you explain it later ) if you feel that people do not know what it means.
Fit needs to be more precise in what it says it works with. It repeatedly refers to function objects and lambda functions as 'functions'. I think this vagueness of terminality is really confusing in the documentation.
Perhaps, I should refer to the Callable concept early on then.
See above.
Quick Start:Function Objects
"We can make it behave like a regular function if we construct the
class
as a global variable."
What about a non-global
sum_f sum = sum_f();
makes 'sum' not behave like a regular function other than the fact that the variable 'sum' may eventually go out of scope ?
In C++, a regular function is always global, there is no such thing as local function(sans gcc extensions).
My point is that the non-global 'sum' in my example above behaves just as much like a regular function as your global 'sum'. You may want to promote the idea of global function objects but I think that this is personally a bad idea. IMO global variables of any kind are to be avoided.
It is quite common in several modern C++ libraries to declare functions as global objects. There are many advantages to this. Why do you believe it should be avoided? Especially since it has the same effect as a free function. DO you believe free function should be avoided as well?
Quick Start:Lambdas
Why do we need both BOOST_FIT_STATIC_LAMBDA and BOOST_FIT_STATIC_LAMBDA_FUNCTION ? I would seem that BOOST_FIT_STATIC_LAMBDA_FUNCTION would be adequate and BOOST_FIT_STATIC_LAMBDA is just redundant, mimicking lambda syntax to
no
purpose.
BOOST_FIT_STATIC_LAMBDA_FUNCTION and BOOST_FIT_STATIC_FUNCTION both define a function at global scope, and can only be used at global scope, whereas BOOST_FIT_STATIC_LAMBDA can be used to constexpr initialize local variables as well.
It might be good to add that when the end-user first encounters BOOST_FIT_STATIC_LAMBDA, because the name does not suggest initialization of local objects. Furthermore I cannot imagine why one would want to use it to initialize a local object,
Really? You just said that you would prefer avoiding global functions.
so you might want to explain the benefit of doing so as opposed to the normal syntax for creating a lambda function.
Maybe the quick start guide isn't the place for this in the first place.
In fact, BOOST_FIT_LIFT uses this since it is not always clear what context the user might call BOOST_FIT_LIFT.
Quick Start:Overloading
The overloading adaptors show two or more lambda functions. Can they also work with function objects ? Or a mix of lambda functions and function objects ? In fact all the reamining Quick Start topics show examples with lambda functions. Do they also work with function objects
?
Yes it can be used with function objects. I probably should show an example of that as well. I used the lambdas because of the terseness of them.
Evidently the adaptors can be used with any Callable. I think you should make that point very strongly and show that in examples also.
Agreed.
Quick Start:Variadic
I do not understand what 'We can also make this print function
varidiac,
so it prints every argument passed into it.' means ?
I'll try to explain that better, but basically it will print each argument, so:
print("hello", 5); // Will print "hello" and 5
I wouldn't associate that with the word 'Variadic' but I do think you need to explain that more clearly.
I always understood variadic to mean taking a variable number of arguments. What do you understand variadic to mean?
I do not think the Quick Start explains very much since it is dealing with adaptors of which we know almost nothing and the explanation for these adaptors and what they actually do is very terse.
Probably can expand the explanation of adaptors a little more.
Good idea. What do adaptors create ?
They create a function.
Different function objects I would imagine ? In that case how about the explanation that adaptors take Callables as input and generate function object types that adapt the original functionality of one or more Callables to some other purpose.
The examples are already show taking Callables. Are you suggesting I show an example taking a member function or something?
In the 'Signatures' section of the Overview I read:
"All the functions are global function objects except where an explicit template parameter is required." I honestly don't know what this is supposed to mean. Does this refer to when function objects are referred to as parameters to the adaptors, functions, and utilities of the
library
?
I don't understand what you are asking. It means that the function is written like this in the documentation:
template<class IntegralConstant> constexpr auto if_(IntegralConstant);
But its actually a function object like this in the code:
struct if_f { template<class IntegralConstant> constexpr auto operator()(IntegralConstant) const; }; const constexpr if_f if_ = {};
However, `if_c` is written like this in the documentation:
template<bool B, class F> constexpr auto if_c(F);
It requires the bool `B` template parameter explicity. So in the code it is written as a function and not as a function object.
I don't understand to what you are referring when you say 'function'. Are you talking about adaptors in your library, functions in your library, or what ?
I am talking about all functions that are defined in the library, that includes adaptors as well. I am not sure how to make that clearer.
Please try to understand that your use of the word 'function' is very broad but that the word 'function' in C++ has a much narrower meaning.
By function, I mean something like in the example: template<class IntegralConstant> constexpr auto if_(IntegralConstant); I think everyone agrees that is a function in C++.
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? - 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. Two of the reasons stated are constexpr and SFINAE support. 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. - 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? - 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). - 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? - 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. - Alias Why is this provided as a utility? What does this have to do with functions? This seems like an implementation detail.
* 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? - 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? - 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? - 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! - 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). - 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? - 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. - 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. - 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.
* 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. - 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. - 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. - 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.
* 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.
* 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?
- 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. Lee
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 Mon, 14 Mar 2016 00:11:31 +0100 "Vicente J. Botet Escriba" <vicente.botet@wanadoo.fr> wrote:
On Thu, 3 Mar 2016 12:43:10 +0100 "Vicente J. Botet Escriba" <vicente.botet@wanadoo.fr> wrote: [snip - everything has been covered in other posts]
* 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?
Le 13/03/2016 23:14, Lee Clagett a écrit : the documentation? Is there something that must be modified so you could accept the library?
I did not respond immediately because I wanted some feedback from Paul, and I had to think about it some more. I do think this library should be included in Boost (vote change by me). A follow-up review of the requested changes and suggestions should be helpful, but I think the Github issue and comment system can do this quite well too. There appears to be a special label for "review", so issues are less likely (than past cases) to be overlooked. The documentation is the part that is likely to suffer on the Github system though; somehow feedback / editing on that should be encouraged. Lee
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) 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.
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
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) 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 ...? 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.
[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.
[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? 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.
// 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? 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
participants (10)
-
Andrey Semashev -
Edward Diener -
Lee Clagett -
Louis Dionne -
paul Fultz -
Paul Fultz II
-
Peter Dimov -
Rob Stewart -
Steven Watanabe -
Vicente J. Botet Escriba