Page MenuHomePhabricator

[libc++][compare] Implement three_way_comparable[_with] concepts
Needs ReviewPublic

Authored by rarutyun on Jun 1 2021, 11:29 AM.

Details

Reviewers
ldionne
cjdb
Mordante
zoecarver
Quuxplusone
jdoerfert
Group Reviewers
Restricted Project
Summary

Implementation of three_way_comparable and three_way_comparable_with concepts from <compare> header.

Please note that I have temporarily removed <compare> header from <utility> due to cyclic dependency that prevents using <concepts> header in <compare> one.

I tried to quickly resolve those issues including applying suggestions from @cjdb and dive deeper by myself but the problem seems more complicated that we thought initially.

I am in progress to prepare the patch with resolving this cyclic dependency between headers but for now I decided to put all that I have to the review to unblock people that depend on that functionality. At first glance the patch with resolving cyclic dependency is not so small (unless I find the way to make it smaller and cleaner) so I don't want to mix everything to one review.

Diff Detail

Event Timeline

rarutyun requested review of this revision.Jun 1 2021, 11:29 AM
rarutyun created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 1 2021, 11:29 AM
cjdb requested changes to this revision.Jun 1 2021, 11:41 AM

Thanks for working on this. To get an LGTM, please add test types for the following:

  • A type where operator<=> isn't declared.
  • A type where operator<=> is deleted.
  • A type where operator<=> is declared and operator== isn't.
  • A type where operator== is deleted.
  • A type where all the inequality operators are defined.
  • A type where all but one of the inequality operators are declared.
  • A type where all but one of the inequality operators are declared, and the missing one is deleted.

PTAL at the totally_ordered and totally_ordered_with tests for inspiration.

libcxx/test/libcxx/inclusions/utility.inclusions.compile.pass.cpp
25–27

Please fix this before merging (same as above).

This revision now requires changes to proceed.Jun 1 2021, 11:41 AM
Quuxplusone requested changes to this revision.Jun 1 2021, 11:42 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp
68

As usual, I don't think it's important to have hundreds of lines of similar/redundant test cases, and I do think it's important to have simple test cases for the corner cases:

  • What if the type provides <=> but no viable ==? And vice versa?
  • What if the type provides <=> with a non-comparison-category return type, such as int?
  • What if the type provides == with a non-boolean return type, such as int? such as void?
  • What if the type provides <=> but it works only on non-const objects?
  • What if the type provides all six C++17 comparison operators but no C++20 <=>?

etc.

libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp
77–80

Nit: Please avoid taking clang-format's advice in cases like this where it's clearly making the formatting worse.
Instead, try treating a clang-format diagnostic as a helpful hint that your lines may be too long. E.g. in this case you don't really need to test a const volatile noexcept abominable function type, right? This test clearly could be rewritten as static_assert(!check_three_way_comparable_with<int, int*>()) (or any similar pair of types; the only important thing here is that the expression a == b would be ill-formed).
Renaming check_three_way_comparable_with to simply check is another good way to improve the readability of this test file while also helping to make clang-format happy.

cjdb added inline comments.Jun 1 2021, 12:09 PM
libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp
68

As usual, the number of tests here are to ensure that all the sharp objects C++ has to offer are checked. I'll say it once again: concepts are a very new feature, and are relatively untested. Our spaceship operator is a a very new feature, and is relatively untested. As far as I'm aware, LLVM has zero other tests that check these two features interact. These tests ensure that nothing has been accidentally overlooked, and are a cheap way for the whole LLVM project to ensure everything is working as intended.

libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp
77–80

I'm sure @rarutyun's employer doesn't pay them to spend time overriding clang-format.

Thanks for working on this.

libcxx/include/compare
487

We typically omit the 'exposition only' comment here.
Can you also update the synopsis with the newly added code. When copying please make sure to remove non-ASCII characters. (The invisible hyphens come to mind.)

494

Please use __ugly_names: a -> __a, b -> __b.

507

Also __ugly_names here.

libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp
30

char8_t isn't available on all platforms, please guard with #ifnef _LIBCPP_HAS_NO_CHAR8_T.

32

Likewise for char16_t and char32_t using #ifndef _LIBCPP_HAS_NO_UNICODE_CHARS

36

To be complete it would be nice to test __int128_t. This should be guarded with #ifndef _LIBCPP_HAS_NO_INT128. __uin128_t uses the same guard.

68

I also like the verbose testing in this case since it's a new feature. I would like to reduce the amount of redundancy, All types here have basically the same three tests

// with default ordering
static_assert(std::three_way_comparable<T>);

// with explicit ordering
static_assert(std::three_way_comparable<T, std::strong_ordering>);
static_assert(std::three_way_comparable<T, std::weak_ordering>);

Can we move this to a template helper function?

zoecarver added inline comments.Jun 1 2021, 3:07 PM
libcxx/include/compare
494

__make_const_lvalue_ref should take care of making this a reference type (i.e., you can remove the "&").

507

Same comment as above.

libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp
78

Do we really need to assert that all these types are not three way comparable? That seems a bit excessive to me (it would be one thing if they were three way comparable, but they're not-- if we use this philosophy for testing, we can have tests of infinite size).

111

Can we get a type where this operator is only available on rvalue references? I think that would currently pass, but should not.

Wmbat added a subscriber: Wmbat.Jun 1 2021, 4:25 PM
cjdb added inline comments.Jun 1 2021, 4:46 PM
libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp
78

Please see my comment above.

Wmbat added a comment.EditedJun 2 2021, 3:20 PM

I would like to bring forward a potential change.

I was looking at how to implement compare_three_way and found out that it needs to be present in both <compare> and <functional> according to the standard draft ( <function> synopsis, <compare> synopsis ). It would mean that compare_three_way would need to be implemented in a different header and then included in both <compare> & <functional>, @cjdb recommends the use of a __compare/compare_three_way.h header

Since compare_three_way depends on the three_way_comparable_with concept ( see here ), we might also need to move three_way_comparable to __compare/compare_three_way.h as well. Doing so now might save us the refactor later down the line.

Any thoughts on this?

I was looking at how to implement compare_three_way and found out that it needs to be present in both <compare> and <functional> according to the standard draft ( <function> synopsis, <compare> synopsis).

Sounds like someone should file an LWG issue. I'm not aware of any other library facilities that are listed multiple times in different places in the Standard. This might be a cut-and-paste error.

tcanens added a subscriber: tcanens.Jun 3 2021, 5:34 PM

I was looking at how to implement compare_three_way and found out that it needs to be present in both <compare> and <functional> according to the standard draft ( <function> synopsis, <compare> synopsis).

Sounds like someone should file an LWG issue. I'm not aware of any other library facilities that are listed multiple times in different places in the Standard. This might be a cut-and-paste error.

This is the new way to say "this thing is provided if you include either header": https://github.com/cplusplus/draft/issues/3683

It's very much intentional that compare_three_way is available if you include either <functional> or <compare>.

Wmbat added a comment.Jun 4 2021, 12:27 PM

I was looking at how to implement compare_three_way and found out that it needs to be present in both <compare> and <functional> according to the standard draft ( <function> synopsis, <compare> synopsis).

Sounds like someone should file an LWG issue. I'm not aware of any other library facilities that are listed multiple times in different places in the Standard. This might be a cut-and-paste error.

This is the new way to say "this thing is provided if you include either header": https://github.com/cplusplus/draft/issues/3683

It's very much intentional that compare_three_way is available if you include either <functional> or <compare>.

Then, my proposition stands. We would have to either make <functional> include all of <compare> or make a separate header that contains everything that compare_three_way needs and have it be included by both <compare> and <functional>

cjdb added a comment.Jun 11 2021, 9:56 AM

Ping. @rarutyun this is blocking other work now; will you have time to work on this next week, or would you prefer I take over?

Ping. @rarutyun this is blocking other work now; will you have time to work on this next week, or would you prefer I take over?

Hi,

Yes, next week is exactly my plan. For this week I have so many deadlines for internal work. But now they are all passed and I can get back to this patch and another one.

rarutyun updated this revision to Diff 353133.Jun 18 2021, 5:17 PM

Partially apply comments. Others are in progress.

Will do my best to apply everything before next work week

rarutyun marked 8 inline comments as done.Jun 18 2021, 5:19 PM
cjdb added a comment.Jun 22 2021, 12:10 PM

I would like to bring forward a potential change.

I was looking at how to implement compare_three_way and found out that it needs to be present in both <compare> and <functional> according to the standard draft ( <function> synopsis, <compare> synopsis ). It would mean that compare_three_way would need to be implemented in a different header and then included in both <compare> & <functional>, @cjdb recommends the use of a __compare/compare_three_way.h header

Since compare_three_way depends on the three_way_comparable_with concept ( see here ), we might also need to move three_way_comparable to __compare/compare_three_way.h as well. Doing so now might save us the refactor later down the line.

Any thoughts on this?

Please move the concepts into their own header and include it in <compare>. You might need to break some other stuff out into their own self-contained headers too.

cjdb added a comment.Jun 29 2021, 5:58 PM

Ping @rarutyun, do you need assistance?

Ping @rarutyun, do you need assistance?

I am sorry for the delay. I was on vacation. I don't think I need assistance so far. I am working hard on that patch this week. If I face some troubles I will definitely let you know.

Thanks for proposing the help.

rarutyun updated this revision to Diff 358092.Mon, Jul 12, 3:51 PM

I would like to bring forward a potential change.

I was looking at how to implement compare_three_way and found out that it needs to be present in both <compare> and <functional> according to the standard draft ( <function> synopsis, <compare> synopsis ). It would mean that compare_three_way would need to be implemented in a different header and then included in both <compare> & <functional>, @cjdb recommends the use of a __compare/compare_three_way.h header

Since compare_three_way depends on the three_way_comparable_with concept ( see here ), we might also need to move three_way_comparable to __compare/compare_three_way.h as well. Doing so now might save us the refactor later down the line.

Any thoughts on this?

Please move the concepts into their own header and include it in <compare>. You might need to break some other stuff out into their own self-contained headers too.

Do you want me to make those APIs available from <functional> in this review?

libcxx/include/utility
228

Still struggling with cyclic dependencies.

cjdb added a comment.Mon, Jul 12, 5:34 PM

Please move your header split into a standalone NFC patch. I'll review it promptly over there.

Please move your header split into a standalone NFC patch. I'll review it promptly over there.

Done. https://reviews.llvm.org/D106107