Some pipes (e.g. filter) can cause their input value to be moved more than once. about pipes

[LouisCharlesC]

Here's a test that makes much more sense to me!

TEST_CASE("filter predicate and pipeline get the input value")
{
	const std::vector<std::vector<int>> original = {{1,2,3,4}, {5,6,7,8}};
	std::vector<std::vector<int>> input = original;

	std::vector<std::vector<int>> valueSeenByThePredicate;
	auto const copyPassthroughFilter = pipes::filter(
		[&valueSeenByThePredicate]
		(std::vector<int> value)
		{
			valueSeenByThePredicate.push_back(value);
			return true;
		});
				
	std::vector<std::vector<int>> results;
	std::copy(std::make_move_iterator(begin(input)), std::make_move_iterator(end(input)), copyPassthroughFilter >>= pipes::push_back(results));

	REQUIRE(std::equal(original.cbegin(), original.cend(), valueSeenByThePredicate.cbegin()));
	REQUIRE(std::equal(original.cbegin(), original.cend(), results.cbegin()));
}

[LouisCharlesC]

Oh, and then the precise issue is not double move, but simply use after move if the predicate moves the value. Whatever the rest of the pipeline does it will do it on moved from values!

And I did sound like you had a choice for the fix, but basically you can only forward at the last use of a variable.
The only flexibility you have is to reorder operations, if you feel it is more beneficial to forward during one operation than the other. Most of the time you don't even have the freedom to reorder, though.

[LouisCharlesC]

Ok, I'll stop the monologue after this, I promise.
To me there is one issue, and 2 ways to solve it.
The issue is: in the current implementation, if the predicate takes its argument by value, the value actually gets moved! This means that a harmless looking predicate taking its argument by value will destroy what is passed to the pipeline tail. That is a surprising effect that must be avoided.
The test to detect this is even more simple than the ones suggested above.

Now the 2 solutions I can suggest are to change line 19 of filter.hpp from
if (predicate_(FWD(value))); to:

1. if (predicate_(value)); or
2. if (predicate_(static_cast<const Value&>(value)));

Both solutions solve the above mentioned issue. They also both fail to compile if the predicate takes the value by rvalue reference (Value&&), which seems correct to me. The only difference is: with solution 2, the predicate cannot take the value by lvalue reference (Value&) either. Solution 1 allows it, and thus the predicate would be allowed to modify the value it is passed. It think it is much less error-prone to go with solution 2. And Pipes being what they are, if you want to modify the value before passing it to the next pipe, you should use a separate pipe to do it, rather than the predicate of a filter pipe.

That's it. I'll be happy to look at the other pipes if you can tolerate 2 full pages of comments for each of my findings :)

[marzojr]

Another thing that could be done is use detection idiom to ensure that the predicates are callable with a const lvalue ref. This had the added advantages of allowing better error messages, and of being similar to what might be done with concepts in the (near?) future.

[joboccara]

Thanks! I went for solution 1 because it seems to me more consistent with the STL.
The fix is in this commit: b79dcfb