This simplifies the usage of __less by making the class not depend on the types compared, but instead the operator(). We can't remove the template completely because we explicitly instantiate std::__sort with __less<T>.
Details
- Reviewers
ldionne Mordante EricWF - Group Reviewers
Restricted Project - Commits
- rG88632e480696: [libc++] Refactor __less
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This cleanup seems incomplete. Why keep __less as a template at all given it's never actually used as one?
Also does the iterator's ::value_type always have to exactly match the type returned from dereferencing it?
libcxx/include/__algorithm/comp.h | ||
---|---|---|
37 | This doesn't at all seem correct to me. This takes away an explicit conversion | |
libcxx/include/__algorithm/includes.h | ||
64 | Is this correct? What if the iterator returns a type convertible to value_type? |
libcxx/include/__algorithm/comp.h | ||
---|---|---|
37 | *The implicit conversion. |
If you block other reviews on there not being a description, at least read it.
libcxx/include/__algorithm/includes.h | ||
---|---|---|
64 | AFAICT the standard it either very silent on this, or says one should use less{} as the comparator, which does what __less<>{} does after this change. So this change is either a conforming change, or arguably a bugfix. Although funnily enough std::less{} isn't actually well-formed, and it doesn't specifically use ranges::less{} for the classic algorithms. |
libcxx/include/__algorithm/comp.h | ||
---|---|---|
32 | So there's actually a behavior change in this patch (the removal of implicit conversions noticed by @EricWF). However, I think it is desirable -- right now I don't think we are conforming in C++20. For example, take lower_bound: http://eel.is/c++draft/lower.bound#1. It specifically says Let comp be less{} and proj be identity{} for overloads with no parameters by those names., which implies that we should be using the transparent comparator, but we are not (before this patch). I think this mandates a change everywhere along with tests. We should also confirm what's the expected behavior in prior Standards and make sure we conform in those as well. | |
33 | Please add a comment explaining why it's empty. |
libcxx/include/__algorithm/comp.h | ||
---|---|---|
32 | I'm not actually convinced this behaviour change is reliably observable. AFAICT it would only be possible to observe with an input_iterator that returns a proxy reference which is comparable to the referenced type. Even then, an implementation is most likely allowed to call both operator<(value_type&, proxy) and operator<(value_type&, value_type&). I'd argue that this case is unspecified behaviour. |
libcxx/include/__algorithm/comp.h | ||
---|---|---|
32 | I am not sure whether this is actually spelled out explicitly in the standard, but it's definitely observable: https://godbolt.org/z/51WKExTxK At the very least, I think we need to provide this guarantee as a QOI, and so we should have tests for that under libcxx/test/libcxx if not in libcxx/test/std. | |
libcxx/src/algorithm.cpp | ||
24 | Clearer IMO |
libcxx/include/__algorithm/sort.h | ||
---|---|---|
895 | ||
901 | ||
908 | ||
916 | ||
libcxx/test/libcxx/algorithms/robust_against_using_non_transparent_comparators.pass.cpp | ||
8 ↗ | (On Diff #527639) | Let's add a comment explaining what this test does, in particular the fact that this is technically not mandated by the standard but we provide it as QOI. |
28 ↗ | (On Diff #527639) |
So there's actually a behavior change in this patch (the removal of implicit conversions noticed by @EricWF). However, I think it is desirable -- right now I don't think we are conforming in C++20. For example, take lower_bound: http://eel.is/c++draft/lower.bound#1. It specifically says Let comp be less{} and proj be identity{} for overloads with no parameters by those names., which implies that we should be using the transparent comparator, but we are not (before this patch).
I think this mandates a change everywhere along with tests. We should also confirm what's the expected behavior in prior Standards and make sure we conform in those as well.