This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [P1614] Implement the second half of [cmp.alg]: compare_{strong,weak,partial}_fallback.
ClosedPublic

Authored by Quuxplusone on Oct 10 2021, 1:36 PM.

Details

Summary

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).

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Oct 10 2021, 1:36 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptOct 10 2021, 1:36 PM

Skip signaling-NaN tests on x86-32 (x87 floating point).
Mark [cmp.alg] as "Complete" in the csv files.

Mordante added inline comments.Oct 11 2021, 10:15 AM
libcxx/docs/Status/SpaceshipProjects.csv
8–13

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/

Quuxplusone marked 3 inline comments as done.Oct 11 2021, 10:31 AM
Quuxplusone added inline comments.
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).

Quuxplusone marked an inline comment as done.

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.

Mordante accepted this revision as: Mordante.Oct 12 2021, 8:52 AM

LGTM but I'd like @ldionne to also have a look.

libcxx/include/__compare/compare_partial_order_fallback.h
56

I read that part of the wording. I was mainly wondering about the fact that both __e and __f can be forwarded 3 times in the return statement. But when you're sure this is fine I'm happy.

Quuxplusone edited the summary of this revision. (Show Details)

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.

libcxx/include/__compare/compare_strong_order_fallback.h
53

@ldionne: Here's one example of a multi-line "you must write it three times" body; let me know what you want this to look like.

Don't static_assert is_iec559 because it's not true on AIX (but the actual test cases will pass anyway).

Rebased. The previous buildkite run was already green, and all comments have been addressed AFAIC. @ldionne ping!

Rebase, poke CI.

ldionne requested changes to this revision.Jan 26 2022, 10:01 AM

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
31

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.

42

And then align the rest.

libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp
1

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.

This revision now requires changes to proceed.Jan 26 2022, 10:01 AM
Quuxplusone marked 2 inline comments as done.

Updated, and removed parent revision so hopefully CI will be happy again.

libcxx/include/__compare/compare_partial_order_fallback.h
31

I have a proposed solution for this in D115607, but won't have time to revisit it before release/14.x.
What I'm doing in D111514 is the same thing I did in std::strong_order etc., so we are consistent between std::strong_order and std::compare_strong_order_fallback. I'm willing (even interested) to revisit these diagnostics post-14.x, but I still think there's value in getting all of [cmp.alg] shipped before the release branch happens.

libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp
1

Ack. I forgot to uncomment the now-existing CPO tests in cpo.compile.pass.cpp. Fixed.

ldionne accepted this revision.Jan 27 2022, 8:55 AM
ldionne added inline comments.
libcxx/include/__compare/compare_partial_order_fallback.h
31

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.

This revision is now accepted and ready to land.Jan 27 2022, 8:55 AM