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

[rarutyun]

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.

[rarutyun]

Partially apply comments. Others are in progress.

Will do my best to apply everything before next work week

[cjdb]

In D103478#2794860, @Wmbat wrote:

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]

Ping @rarutyun, do you need assistance?

[rarutyun]

In D103478#2848944, @cjdb wrote:

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]

In D103478#2834101, @cjdb wrote:

In D103478#2794860, @Wmbat wrote:

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 ↗

(On Diff #358092)

Still struggling with cyclic dependencies.

[cjdb]

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

[rarutyun]

In D103478#2872748, @cjdb wrote:

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

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

[mumbleskates]

This is probably unblocked now that D107584 has landed!

[rarutyun]

In D103478#2938752, @mumbleskates wrote:

This is probably unblocked now that D107584 has landed!

Thanks, will check today

[• Quuxplusone]

In D103478#2939873, @rarutyun wrote:

In D103478#2938752, @mumbleskates wrote:

This is probably unblocked now that D107584 has landed!

Thanks, will check today

Btw, @rarutyun, if you'd like me to commandeer and replace this PR with the relevant portion of D107036, I'd be happy to do that.

[rarutyun]

In D103478#2939873, @rarutyun wrote:

In D103478#2938752, @mumbleskates wrote:

This is probably unblocked now that D107584 has landed!

Thanks, will check today

Checked. Locally it works. Review has been updated.

[rarutyun]

In D103478#2940587, @Quuxplusone wrote:

In D103478#2939873, @rarutyun wrote:

In D103478#2938752, @mumbleskates wrote:

This is probably unblocked now that D107584 has landed!

Thanks, will check today

Btw, @rarutyun, if you'd like me to commandeer and replace this PR with the relevant portion of D107036, I'd be happy to do that.

Whatever works for you. You may say to me what should be applied.

[rarutyun]

Clang-format suggests incorrect modifications that fail to compile if applied. Could you please suggest the file to look at for the best practice how to turn it off? I could you dedicated comment for that but I am not sure that's how you guys are doing that.

[cjdb]

In D103478#2941860, @rarutyun wrote:

Clang-format suggests incorrect modifications that fail to compile if applied. Could you please suggest the file to look at for the best practice how to turn it off? I could you dedicated comment for that but I am not sure that's how you guys are doing that.

Run arc diff --nolint instead of arc diff.

[• Quuxplusone]

@rarutyun: I've marked the places where this is lacking bits you can take from D107036.
Re the testing issue, let's see what @ldionne wants.

libcxx/include/CMakeLists.txt

106

You'll also need to update module.modulemap and run the various Python generator scripts (which you can now do via ninja libcxx-generate-files or make libcxx-generate-files). Make sure to commit the changed and added new files.

libcxx/include/__compare/three_way_comparable.h

13–17

You're missing some IWYU here, e.g. same_as.h. You need to include these directly, for the benefit of the Modules build; it's not Modules-kosher to rely on the fact that totally_ordered.h happens to include same_as.h.
Check D107036 for the complete listing.

libcxx/include/compare

30–36

Right about here, please update the synopsis comment.
Check D107036 for the right thing.

140

a-z

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

77–80

@rarutyun, you wrote:

Clang-format suggests incorrect modifications that fail to compile if applied.

For generating the diff to upload, I use git show -U999 > x.diff and then "Upload file" into the Phab review. I don't use arc at all in my personal workflow. I don't know if this will help you, but at least I personally spend zero time wrestling with arc; some amount of time wrestling with git instead; and zero time wrestling with clang-format because clang-format is completely unused in my git workflow.

Re Chris's snipe: Don't worry about clang-format. Just write the code the way you'd write it naturally, and then act on Phab feedback (if any) from human libc++ maintainers. clang-format isn't the boss here. :)

Also, note that a massive number of these 230+242 lines can disappear entirely. I tentatively recommend just taking my 101+147 lines of more targeted testing from D107036 (although let's see if @ldionne has a strong preference for these, or wants both, or what). Either way, please delete or fix the misformatting throughout this file.

[cjdb]

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

77–80

Also, note that a massive number of these 230+242 lines can disappear entirely. I tentatively recommend just taking my 101+147 lines of more targeted testing from D107036 (although let's see if @ldionne has a strong preference for these, or wants both, or what). Either way, please delete or fix the misformatting throughout this file.

See https://reviews.llvm.org/D103478#inline-980152.

[• Quuxplusone]

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

50

This specific line is failing on AArch64 and Arm. I suspect that those platforms have a 32-bit unsigned wchar_t, and so this line would be failing for the same reason that

static_assert(std::three_way_comparable<int, unsigned int>);

would fail. Therefore, the correct action is to remove this platform-dependent line (and lines 55 and 59 too). However, consider adding

static_assert(!std::three_way_comparable<int, unsigned int>);

as a sanity check; I think you're missing anything along those lines at the moment.

[• Quuxplusone]

libcxx/include/__compare/three_way_comparable.h

35–36

requires(__make_const_lvalue_ref<_Tp> __t) {
    { __t <=> __t } -> __compares_as<_Cat>;

Let's keep it simple (and close to the reference implementation).

45

Let's eliminate this linebreak.

[cjdb]

libcxx/include/__compare/three_way_comparable.h

28

This comment is unnecessary.

35–36

We shouldn't deviate from what's in the standard. The concept is defined the way it is way for a reason.

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

50

Interesting. I'd prefer to section this test off using an include guard, if it's possible.

[rarutyun]

libcxx/include/CMakeLists.txt

106

Yep, I figured it out from CI results. Done

libcxx/include/__compare/three_way_comparable.h

35–36

Agree with Christopher

[• Quuxplusone]

LGTM % comments at this point.
I'm still unhappy with the testing story — >200 lines of "this kind of member function pointer isn't comparable, that kind of member function pointer isn't comparable..." — but since the code is correct, I'd rather land this as-is than continue with review.

libcxx/include/__compare/three_way_comparable.h

35–36

Ah, I see the reference implementation actually does use a and b; I must have simplified it on my end when I did D107036. Okay then.
(I do disagree that "the concept is defined the way it is for a reason"; it could have been done simpler without b, and had exactly the same semantics. But that's moot until someone files an editorial issue.)

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

111

remove linebreak plz

229

} // namespace user_defined

231

Please remove the main function. For .compile.pass.cpp tests, which aren't intended to be run (just compiled), we've settled on making sure they don't have a main.

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

50

Nah, just delete the bogus line.

122

} // namespace fundamentals
(and please fix the linebreaks throughout the above; thanks for taking care of the other file already)

241

} // namespace user_defined

243

and remove main

[• Quuxplusone]

@rarutyun anything blocking this? Do you need help landing it?

[rarutyun]

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

77–80

Avoiding clang-format. Now everything is written in one line and I agree that it looks better. I would not want to rename check_three_way_comparable_with to just check because I believe that decreases readability but if you insist I may do that.

I am not sure about reducing tests. Seems like different people have different opinions on that. I would prefer to leave it as is unless you have strong objections.

[rarutyun]

In D103478#2951267, @Quuxplusone wrote:

@rarutyun anything blocking this? Do you need help landing it?

Review has been updated. Just was busy with other work.

[rarutyun]

Hi @cjdb @Quuxplusone. Any progress on this review?

I think I've addressed all important comments and the CI is green (except the clang format, which is not a boss here:) ).

[• Quuxplusone]

@rarutyun: Ship it!

[cjdb]

Also LGTM, but since I'm not a co-approver, we'll need to wait for either @ldionne's approval or a second co-approver.

libcxx/include/__compare/three_way_comparable.h

13–17

For future reference: running bin/llvm-lit -sv /path/to/llvm-project/libcxx/test/ --param enable_modules=True will catch this for you.

[rarutyun]

In D103478#2960589, @cjdb wrote:

Also LGTM, but since I'm not a co-approver, we'll need to wait for either @ldionne's approval or a second co-approver.

Ok, Waiting for another required approval.

By the way as you might remember I don't have direct access to merge the code directly.

I am putting my data here:

name: Ruslan Arutyunyan
email: [e-mail]

Please let me know if you want me to get the write access directly into the repository.

[ldionne]

I'll commit this. Thanks a lot for the patch.

[ldionne]

@rarutyun Can you please rebase this onto main, it doesn't apply cleanly?

[ldionne]

In D103478#2961333, @rarutyun wrote:

Please let me know if you want me to get the write access directly into the repository.

Yes, that would be useful!

[rarutyun]

In D103478#2965197, @ldionne wrote:

@rarutyun Can you please rebase this onto main, it doesn't apply cleanly?

@ldionne,

Sure, done.

[rarutyun]

Rebased one more time.

@cjdb, @Quuxplusone @ldionne, could somebody merge it please?

My data:
name: Ruslan Arutyunyan
email: [e-mail]

I will request the permissions for direct merge for the next commit if you don't mind.

[ldionne]

In D103478#2972085, @rarutyun wrote:

Rebased one more time.

@cjdb, @Quuxplusone @ldionne, could somebody merge it please?

My data:
name: Ruslan Arutyunyan
email: [e-mail]

I will request the permissions for direct merge for the next commit if you don't mind.

Yes, please do so. Thanks @mumbleskates for merging.