The tests for these are just copy-pasted from the tests for std::{strong,weak,partial}_order, and then I added an extra clause in each (test_2()) to test the stuff that's not just the same as std::*_order.
This also includes the fix for https://wg21.link/LWG3465 (which falls naturally out of the "you must write it three times" style, but I've added test cases for it also).
Details
Details
- Reviewers
ldionne rarutyun jloser Mordante - Group Reviewers
Restricted Project - Commits
- rGbf20a09790cb: [libc++] [P1614] Implement the second half of [cmp.alg]: compare_{strong,weak…
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Skip signaling-NaN tests on x86-32 (x87 floating point).
Mark [cmp.alg] as "Complete" in the csv files.
| libcxx/docs/Status/SpaceshipProjects.csv | ||
|---|---|---|
| 10 ↗ | (On Diff #378556) | Please add a note that long double isn't implemented yet. |
| libcxx/include/__compare/compare_partial_order_fallback.h | ||
| 56 | The same argument can be forwarded multiple times. Can that give an issue with some evil rvalue comparison operators? | |
| libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp | ||
| 47 | s/weak_order/partial_order/ | |
| libcxx/include/__compare/compare_partial_order_fallback.h | ||
|---|---|---|
| 56 | Physically, yes. Semantically, no, either because that would be UB (violating some semantic requirement or other), or because that would be completely expected and documented behavior — notice that http://eel.is/c++draft/cmp.alg#4 says compare_strong_order_fallback(E, F) is expression-equivalent to exactly this expression, "except that E and F are evaluated only once" (which is Standardese for "they're function arguments, not macro arguments," so e.g. std::compare_strong_order_fallback(++i, j) does one increment of i, not up-to-three increments, no matter how the comparison turns out). | |
Comment Actions
strong_order depends on compare_three_way depends on three_way_comparable_with depends on Concepts, so UNSUPPORTED: libcpp-no-concepts and guard the header too.
Comment Actions
Rebase after landing D110738. Rename __e/__f to __t/__u. Remove _LIBCPP_HAS_NO_SPACESHIP_OPERATOR. Adjust spacing on "you must write it three times"... except, @ldionne, you'll have to tell me what you want to do with the several multi-line "three times" bodies in this PR! I've left them alone for now, pending your preferred style.
Comment Actions
Don't static_assert is_iec559 because it's not true on AIX (but the actual test cases will pass anyway).
Comment Actions
Rebased. The previous buildkite run was already green, and all comments have been addressed AFAIC. @ldionne ping!
Comment Actions
The logic looks alright to me, but I have a significant comment about the approach taken and its impact on diagnostics.
| libcxx/include/__compare/compare_partial_order_fallback.h | ||
|---|---|---|
| 32 | I have already voiced concerns with this approach to constraining functions: it produces terrible diagnostics, and that's a bug. For example: .../cmp/cmp.alg/compare_partial_order_fallback.pass.cpp:315:5: error: no matching function for call to object of type 'const std::__compare_partial_order_fallback::__fn'
std::compare_partial_order_fallback(b, b);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/c++/v1/__compare/compare_partial_order_fallback.h:58:46: note: candidate template ignored: substitution failure [with _Tp = N12::B &, _Up = N12::B &]: no matching function for call to '__go'
_LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t, _Up&& __u) const
^
1 error generated.That's it, nothing else. There's no backtrace at all, so it's impossible to figure out what we did wrong when calling the function. I don't know why it's so bad, I thought that even SFINAE-based failures had some backtrace, but it looks like that's not the case here. I know I'm really annoying with this requirement, and I know it's painful and C++ doesn't give us good tools to achieve both correctness and good diagnostics. It's actually embarrassing to see how difficult it is to be both correct and user-friendly with concepts, when that was the whole point of the feature. I don't know whether that's a property of the feature itself or if it's just Clang doing a really bad job at merging SFINAE failures and concepts failures, but the result is the same, and it's terrible. However, what I do know is that we don't want libc++ to become the "C++ Standard Library with crappy diagnostics". Clang and libc++ have always strived to provide a good user experience, and it's actually an important property to maintain. I don't care super strongly about how this is implemented, but I'd like to have reasonable diagnostics. Not even great, but something a user can work with. The usual way to provide this is to flatten the constraints into a single operator() overload set. The constraints become more complicated because they need to be mutually exclusive, but it works and it gives decent diagnostics. | |
| 43 | And then align the rest. | |
| libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp | ||
| 2 | Unless I missed it, I don't think we're testing that these are CPOs anywhere. Can we add tests for this? Perhaps they can be grouped similarly to what we're doing for ranges. | |
Comment Actions
Updated, and removed parent revision so hopefully CI will be happy again.
| libcxx/include/__compare/compare_partial_order_fallback.h | ||
|---|---|---|
| 32 | I have a proposed solution for this in D115607, but won't have time to revisit it before release/14.x. | |
| libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp | ||
| 2 | Ack. I forgot to uncomment the now-existing CPO tests in cpo.compile.pass.cpp. Fixed. | |
| libcxx/include/__compare/compare_partial_order_fallback.h | ||
|---|---|---|
| 32 | Yes, I noticed this is the same that you did in std::strong_order, and I wish I had caught that when the review for strong_order happened. Ok, let's not block this for the release branch, however please go back and provide decent diagnostics for both strong_order (etc..) and these ones. I'm pretty serious about diagnostics not sucking, it's an important property for users. It's more important to provide good diagnostics to our thousands (?) of users than to have an implementation that looks nicer from our libc++ developer's perspective, when the two are within the same ballpark in terms of complexity. | |
I have already voiced concerns with this approach to constraining functions: it produces terrible diagnostics, and that's a bug. For example:
.../cmp/cmp.alg/compare_partial_order_fallback.pass.cpp:315:5: error: no matching function for call to object of type 'const std::__compare_partial_order_fallback::__fn' std::compare_partial_order_fallback(b, b); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/c++/v1/__compare/compare_partial_order_fallback.h:58:46: note: candidate template ignored: substitution failure [with _Tp = N12::B &, _Up = N12::B &]: no matching function for call to '__go' _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t, _Up&& __u) const ^ 1 error generated.That's it, nothing else. There's no backtrace at all, so it's impossible to figure out what we did wrong when calling the function. I don't know why it's so bad, I thought that even SFINAE-based failures had some backtrace, but it looks like that's not the case here.
I know I'm really annoying with this requirement, and I know it's painful and C++ doesn't give us good tools to achieve both correctness and good diagnostics. It's actually embarrassing to see how difficult it is to be both correct and user-friendly with concepts, when that was the whole point of the feature. I don't know whether that's a property of the feature itself or if it's just Clang doing a really bad job at merging SFINAE failures and concepts failures, but the result is the same, and it's terrible.
However, what I do know is that we don't want libc++ to become the "C++ Standard Library with crappy diagnostics". Clang and libc++ have always strived to provide a good user experience, and it's actually an important property to maintain. I don't care super strongly about how this is implemented, but I'd like to have reasonable diagnostics. Not even great, but something a user can work with.
The usual way to provide this is to flatten the constraints into a single operator() overload set. The constraints become more complicated because they need to be mutually exclusive, but it works and it gives decent diagnostics.