This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement `operator<=>` for `filesystem::path`
ClosedPublic

Authored by avogelsgesang on Jul 31 2022, 3:50 PM.

Details

Summary

Implements part of P1614R2 "The Mothership has Landed"

Diff Detail

Event Timeline

avogelsgesang created this revision.Jul 31 2022, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 3:50 PM
avogelsgesang requested review of this revision.Jul 31 2022, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 3:50 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
avogelsgesang added inline comments.Jul 31 2022, 3:51 PM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp
28

clang-format proposed this change when posting the commit for review. Please let me know if you prefer me to undo it

avogelsgesang planned changes to this revision.Aug 1 2022, 2:48 PM

need to update based on comments in https://reviews.llvm.org/D130852

Update based on feedback from D130852

fix clang-format CI failure (I don't understand why it wants this indentation - but I don't care to fit it...)

SGTM modulo some nits.

libcxx/include/filesystem
165

Can you update the synopsis to make them friends?

libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp
118

Maye use the test macros.

118

Can you test for the proper return type too?

avogelsgesang added inline comments.Aug 13 2022, 5:41 PM
libcxx/include/filesystem
165

not sure where I should put them, if I change them to be friends.

The path class is only forward-declared in the synopsis.
As friends, I should probably move them into the path class.
Does that mean, that I should simply delete the comparisons from the synopsis here, thereby being consistent with the other members of path which are also not shown in the synopsis?

Mordante added inline comments.Aug 14 2022, 7:23 AM
libcxx/include/filesystem
165

Path has its own synopsis hidden in http://eel.is/c++draft/fs.class.path.general#6

We could add that to the main synopsis. We've done that in other places too, but not in a consistent fashion.
(I know updating the synopsis is a bit of a hassle.)

avogelsgesang marked 2 inline comments as done.

address review comment

avogelsgesang added inline comments.Aug 15 2022, 3:21 PM
libcxx/include/filesystem
165

I have added a synopsis for class path now. Please let me know if this is what you had in mind

simplify test cases

add missing include

fix CI for real

Mordante accepted this revision.Aug 17 2022, 9:40 AM

LGTM, thanks!

libcxx/include/filesystem
17–156

We normally don't do this, but it's interesting to do this for those "hidden" synopsis.
(I get the impression their number grows.)

This revision is now accepted and ready to land.Aug 17 2022, 9:40 AM
avogelsgesang marked 2 inline comments as done.Aug 17 2022, 10:40 AM
avogelsgesang added inline comments.
libcxx/include/filesystem
17–156

is your comment an ask to remove this and follow the established way of not including this?
Or does your " it's interesting to do this for those "hidden" synopsis" imply that it's interesting/useful enough so that we should keep this comment?

Mordante added inline comments.Aug 18 2022, 9:00 AM
libcxx/include/filesystem
17–156

Sorry if I was unclear.

I think it's useful and we might even consider doing that for other "hidden" synopsis too. That is sometimes useful during review. I noticed with ranges it's sometimes not easy to find where to look in the Standard.