Details
- Reviewers
Mordante philnik - Group Reviewers
Restricted Project - Commits
- rG882fba9ff275: [libc++][ranges] Implement LWG-3865 Sorting a range of pairs
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for your patch! I haven't looked in detail, since I first like to agree to which language versions we want to backport this fix.
| libcxx/test/std/utilities/tuple/tuple.tuple/tuple.apply/make_from_tuple.pass.cpp | ||
|---|---|---|
| 68 | We typically back-port LWG issues to older language versions. Since operator== and operator<=> are added/changed in C++20, I feel less wary to change it there. @ldionne What's your opinion? | |
| libcxx/test/std/utilities/utility/pairs/pairs.spec/comparison.pass.cpp | ||
| 11 | Can you also update libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp. | |
| libcxx/test/std/utilities/tuple/tuple.tuple/tuple.apply/make_from_tuple.pass.cpp | ||
|---|---|---|
| 68 | The changes do break such not-so-reasonable code: struct BadPairs : std::pair<int, int>, std::pair<long, long> {};
std::pair<int, int>{} == BadPairs{};And some uses of (IMO, unreasonable) explicit usage of operartor@(p, x) or operator@<T, U>(p, x). I think if we provide both old and new overloads, almost nothing is broken (except some code expecting incomparability). | |
Given that the MSVC-STL team is planning on backporting this fix to C++14 (the earliest supported version), I think it's reasonable to backport this to at least C++11 (see the comment thread).
I think it's good if MSVC-STL and libc++ are aligned on this.
| libcxx/test/std/utilities/tuple/tuple.tuple/tuple.apply/make_from_tuple.pass.cpp | ||
|---|---|---|
| 68 | Given that you aren't allowed to explicitly specify template parameters unless the standard explicitly allows it, I think we can break at least that part. I don't really expect a lot of code to be broken in the wild, so I think it should be OK to back-port to C++03. Since we are early in the release cycle we can also just land it for now; if enough people complain we can revert. | |
I think we should implement this as a DR in all supported modes, like any normal LWG issue. I would only change this stance if we find actual breakage in the wild of crazy incompatibilities with older standard modes.
| libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp | ||
|---|---|---|
| 13–15 | Do you not need actual changes to the tests below? | |
| libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp | ||
|---|---|---|
| 13–15 | Yes, thanks, I added tests. | |
In general LGTM, but one comment.
| libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp | ||
|---|---|---|
| 43 | I would like to see a few more tests int <=> double, int <=> string. | |
| libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp | ||
|---|---|---|
| 43 | I added int <=> double But what test do you want with int <=> string ? | |
| libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp | ||
|---|---|---|
| 43 | Making sure it does not work, so this needs to be some static_assert. | |
The following snippet (reduced from a version of mysql) compiles well before this commit and fails to compile after it (https://gcc.godbolt.org/z/zrGbbbWb5):
#include <utility>
struct X {};
struct Y {};
bool operator==(const std::pair<const X, const X> &a, const std::pair<Y, Y> &b);
bool f(std::pair<const X, X> x, std::pair<Y, Y> y) {
return x == y;
}The error is:
In file included from bin/../include/c++/v1/utility:259:
bin/../include/c++/v1/__utility/pair.h:432:22: error: invalid operands to binary expression ('const X' and 'const Y')
return __x.first == __y.first && __x.second == __y.second;
~~~~~~~~~ ^ ~~~~~~~~~
<source>:9:12: note: in instantiation of function template specialization 'std::operator==<const X, X, Y, Y>' requested here
return x == y;
^
<source>:6:6: note: candidate function not viable: no known conversion from 'const X' to 'const std::pair<const X, const X>' for 1st argument
bool operator==(const std::pair<const X, const X> &a, const std::pair<Y, Y> &b);
^
bin/../include/c++/v1/__utility/pair.h:430:1: note: candidate template ignored: could not match 'const pair<_T1, _T2>' against 'const X'
operator==(const pair<_T1,_T2>& __x, const pair<_U1,_U2>& __y)
^I can fix this by changing the custom operator== declaration to bool operator==(const std::pair<const X, X> &a, const std::pair<Y, Y> &b); (removing the second const), but I wonder if this sort of a breakage is intended.
We typically back-port LWG issues to older language versions.
However I'm a bit wary of retroactively applying this LWG issue, since it may affect existing code.
Looking at MSVC STL it seems @strega-nil has the same concerns.
https://github.com/microsoft/STL/pull/3476/files/4f2ec495a6b5e6ab61d504663b0ad7d3db9db547#r1110452521
Since operator== and operator<=> are added/changed in C++20, I feel less wary to change it there.
Doing this back to C++03 feels a bit scary.
@ldionne What's your opinion?