This is an archive of the discontinued LLVM Phabricator instance.

[libc++][spaceship] Implement `operator<=>` for `time_point`
ClosedPublic

Authored by H-G-Hristov on Mar 16 2023, 11:03 AM.

Details

Summary

Depends on D145881

Implements parts of P1614R2: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1614r2.html
Implements operator<=> for time_point

Diff Detail

Event Timeline

H-G-Hristov created this revision.Mar 16 2023, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 11:03 AM
H-G-Hristov requested review of this revision.Mar 16 2023, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 11:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
  • Updated URL
  • Updated test
H-G-Hristov edited the summary of this revision. (Show Details)Mar 17 2023, 5:17 PM
  • Updated test

In general LGTM modulo some nits.

libcxx/test/std/time/time.point/time.point.comparisons/compare.three_way.pass.cpp
23

Pedantic, but this include is mandated not to be required. The chrono header is required to include this header.
(http://eel.is/c++draft/time.syn)

32

I would prefer to have some test where Duration::rep is a floating-point type.

H-G-Hristov planned changes to this revision.Mar 22 2023, 2:19 PM

In general LGTM modulo some nits.

Thank you for the review. I'll do that.

H-G-Hristov planned changes to this revision.Mar 22 2023, 2:42 PM

Updated tests

H-G-Hristov marked 2 inline comments as done.Mar 24 2023, 5:13 AM
H-G-Hristov edited the summary of this revision. (Show Details)
Mordante added inline comments.Mar 24 2023, 9:59 AM
libcxx/test/std/time/time.point/time.point.comparisons/compare.three_way.pass.cpp
61

Instead of one big test I would suggest to use 3 smaller tests and call these 3 test from this test function.

That makes is clearer which 3 parts you are testing.

  • Addressed comments
H-G-Hristov marked an inline comment as done.Mar 24 2023, 11:46 AM

Thank you for the review. I think I addressed the suggestion.

Rebased to run CI

Mordante accepted this revision.Apr 8 2023, 5:22 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Apr 8 2023, 5:22 AM

Thanks, LGTM!

Thank you for the review!