ericniebler / range-v3

Range library for C++14/17/20, basis for C++20's std::ranges
Other
4.06k stars 437 forks source link

ranges::actions::unique incorrect behavior with projections #1756

Closed eroller closed 1 year ago

eroller commented 1 year ago

The following test fails.

TEST(RangeV3, PairMemberActionUnique)
{
  auto           x = std::vector<std::pair<int,int>>{{1, 2}, {1, 3}, {1, 3}};
  x |= ranges::actions::unique(ranges::less{}, &std::pair<int,int>::second);

  std::vector<std::pair<int,int>> expected = {{1, 2}, {1, 3}};
  ASSERT_EQ(expected, x);
}
      Expected: expected
      Which is: { (1, 2), (1, 3) }
To be equal to: x
      Which is: { (1, 2) }

Also, I noticed that ranges::views::unique doesn't support projections: https://github.com/ericniebler/range-v3/blob/master/include/range/v3/view/unique.hpp#L56 Is there a fundamental reason it could not?

brevzin commented 1 year ago

This is undefined behavior. unique requires an equivalence relation, and less{} is not one. range-v3 uses sortable as the constraint here, which isn't right:

https://github.com/ericniebler/range-v3/blob/74e44001bb6fac8d7b548a4b717b9b834c834ad9/include/range/v3/algorithm/unique.hpp#L41-L52

The standard library has indirect_equivalence_relation for this case. PRs welcome to fix.

Is there a fundamental reason it could not?

Fundamental? No. But it's easy to project a binary function with a function adapter on(f, proj), which seems better than changing every algorithm that takes a binary function to figure out how to add a projection to it.

eroller commented 1 year ago

This makes sense. Thanks for the quick reply!