[libcxx-commits] [PATCH] D130515: [libc++][ranges] Make sure all range algorithms support differing projection types:

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

In D130515#3681039 <https://reviews.llvm.org/D130515#3681039>, @var-const wrote:

> In D130515#3678127 <https://reviews.llvm.org/D130515#3678127>, @huixie90 wrote:
>
>> The fix looks good to me and I think we should delete  the overload `__make_projected_comp(__comp, __proj1, __proj2)` and replace all the usages with hand coded `std::invoke`
>
> Without this test, I'd agree. With the test, however, I think the benefits outweigh the drawbacks. If an algorithm switches the comparison order, this test will break. However, `make_projected_comp` not only reduces on boilerplate and keeps most internal algorithms unaware of projections, it also allows passing in the original comparator without modification and "eliding" `identity` when possible.

Yes I do agree in general it keeps things tidy. I only have concerns with this particular overload that takes two projections. The problem is that the algorithm's concept constrained the `__comp` to be able to call `__comp(*in1, *in1)`, `__comp(*in1, *in2)`, `__comp(*in2, *in1)`, `__comp(*in2, *in2)`.  
For example, https://en.cppreference.com/w/cpp/iterator/mergeable which calls https://en.cppreference.com/w/cpp/concepts/relation

But this particular overload assumes it always calls `__comp(*in1, *in2)`. By having this overload available, in the future it could break things if we add new algorithms or even just use it internally


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130515/new/

https://reviews.llvm.org/D130515