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