Some pipes (e.g. filter) can cause their input value to be moved more than once.
LouisCharlesC opened this issue · 5 comments
The onReceive member function of the filter pipe forwards their input value both to the predicate_ and to the tailPipeline. This can lead to the value being moved more than once if both take the argument by value or &&.
Some other pipes also forward their input value more than once.
See below for a test you can add to filter to detect the issue.
I think the fix for filter is to not forward to the predicate, as predicates are likely to take their arguments by const&.
For other pipes, you may want to think about where it is more likely that forwarding will serve.
class TwoMoveDetector
{
public:
TwoMoveDetector() = default;
TwoMoveDetector(const TwoMoveDetector& other) = default;
TwoMoveDetector(TwoMoveDetector&& other)
{
REQUIRE_FALSE(other.m_movedFrom);
other.m_movedFrom = true;
}
private:
bool m_movedFrom = false;
};
TEST_CASE("filter moves value at most once")
{
std::vector<TwoMoveDetector> input(1);
auto const passthrough = pipes::filter([](TwoMoveDetector){return true;});
std::vector<TwoMoveDetector> results;
std::copy(std::make_move_iterator(begin(input)), std::make_move_iterator(end(input)), passthrough >>= pipes::push_back(results));
}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()));
}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.
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:
if (predicate_(value));orif (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 :)
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.