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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

In D130515#3681057 <https://reviews.llvm.org/D130515#3681057>, @huixie90 wrote:

> 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

Yes, but then the new test from this patch would break, right? I guess if we add a new algorithm, then we have to remember to update this test if appropriate, which is a downside. I still think that if we can verify that the projected comparator is only used with the non-reversed order of arguments, it's better to use it.

If you can think of a case where we could use the projected comparator incorrectly and this test would *not* break, let me know -- that would be pretty bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130515