Page MenuHomePhabricator

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

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

Details

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.Jul 12 2021, 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 ↗(On Diff #358092)

Still struggling with cyclic dependencies.

cjdb added a comment.Jul 12 2021, 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

This is probably unblocked now that D107584 has landed!

This is probably unblocked now that D107584 has landed!

Thanks, will check today

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 updated this revision to Diff 366019.Aug 12 2021, 9:30 AM
rarutyun set the repository for this revision to rG LLVM Github Monorepo.

This is probably unblocked now that D107584 has landed!

Thanks, will check today

Checked. Locally it works. Review has been updated.

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 updated this revision to Diff 366037.Aug 12 2021, 11:04 AM

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 added a comment.Aug 12 2021, 11:12 AM

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 requested changes to this revision.Aug 12 2021, 3:09 PM

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

This revision now requires changes to proceed.Aug 12 2021, 3:09 PM
cjdb added inline comments.Aug 12 2021, 3:15 PM
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.

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.

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 added inline comments.Aug 12 2021, 10:16 PM
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 updated this revision to Diff 366266.Aug 13 2021, 6:32 AM
rarutyun marked 7 inline comments as done.
rarutyun added inline comments.
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

rarutyun marked an inline comment as done.Aug 13 2021, 6:34 AM
rarutyun updated this revision to Diff 366289.Aug 13 2021, 8:17 AM
Quuxplusone accepted this revision.Aug 13 2021, 9:01 AM

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

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

rarutyun updated this revision to Diff 367195.Aug 18 2021, 6:35 AM
rarutyun marked 7 inline comments as done.Aug 18 2021, 6:44 AM
rarutyun added inline comments.
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 updated this revision to Diff 367196.Aug 18 2021, 6:46 AM
rarutyun marked 2 inline comments as done.Aug 18 2021, 6:49 AM

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

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

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

cjdb accepted this revision.Aug 23 2021, 11:15 AM

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.

This revision is now accepted and ready to land.Aug 23 2021, 11:15 AM

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: ruslan.arutyunyan@intel.com

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

ldionne accepted this revision.Aug 25 2021, 8:36 AM

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

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

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

Yes, that would be useful!

rarutyun updated this revision to Diff 368853.EditedAug 26 2021, 5:06 AM

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

@ldionne,

Sure, done.

rarutyun updated this revision to Diff 369417.Aug 30 2021, 5:07 AM

Rebased one more time.

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

My data:
name: Ruslan Arutyunyan
email: ruslan.arutyunyan@intel.com

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

Rebased one more time.

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

My data:
name: Ruslan Arutyunyan
email: ruslan.arutyunyan@intel.com

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.