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

[H-G-Hristov]

Summary

Implements parts of P1614R2
Implemented operator<=> for std::chrono::duration

[H-G-Hristov]

@Mordante I decided to work on <=> for duration and time_point. I see these are assigned to you. Sorry if I am interfering with your plans, feel free to reject this patch if necessary.

[H-G-Hristov]

• Fixed revision

[H-G-Hristov]

• Minor tweak

[H-G-Hristov]

• Missing include

[H-G-Hristov]

• Fix CI

[H-G-Hristov]

• Minor tweak

[Mordante]

Thanks for working on this! A few comments.

libcxx/docs/Status/SpaceshipProjects.csv

55

Please reach out to us on Discord when you want to work on patches that are assigned to people, me in this case.
Some of the patches in this list already have patches locally that are block for various reasons.

libcxx/test/std/time/time.duration/time.duration.comparisons/compare.three_way.pass.cpp

27

Please add runtime tests too.
I'm quite sure you already used our

test();
static_assert(test());

pattern in other patches. If not I can point you to examples.

66

Nit I think it's safer to use less or greater when comparing floating point values.

[H-G-Hristov]

In D145881#4199876, @Mordante wrote:

Thanks for working on this! A few comments.

Thank you for the review. I'll make sure to contact you first in the future as you recommend.
Sorry if working on this and on time_point D146250 was inappropirate.

[H-G-Hristov]

• Updated tests

[Mordante]

In D145881#4200625, @H-G-Hristov wrote:

In D145881#4199876, @Mordante wrote:

Thanks for working on this! A few comments.

Thank you for the review. I'll make sure to contact you first in the future as you recommend.
Sorry if working on this and on time_point D146250 was inappropirate.

The main thing we want to avoid is having multiple people working on the same feature.
Somethings something in the list is assigned and appears not to be moving, but there might be reasons.
For example parts of the regex spaceship I've been working on, but it was blocked.

I've not started on duration and timepoint, so feel free to implement them.

If you want to look at other things for the spaceship operator. I *think* not all items of paper P1614 are on the list, specifically the removal of operator!=, which will be automatically generated.
If you want to update the status page with these missing entries it would be very much appreciated.

libcxx/test/std/time/time.duration/time.duration.comparisons/compare.three_way.pass.cpp

29

I would remove the constexpr here, I don't think it's needed.

[H-G-Hristov]

• Updated tests

[H-G-Hristov]

In D145881#4202992, @Mordante wrote:

In D145881#4200625, @H-G-Hristov wrote:

In D145881#4199876, @Mordante wrote:

Thanks for working on this! A few comments.

Thank you for the review. I'll make sure to contact you first in the future as you recommend.
Sorry if working on this and on time_point D146250 was inappropirate.

The main thing we want to avoid is having multiple people working on the same feature.
Somethings something in the list is assigned and appears not to be moving, but there might be reasons.
For example parts of the regex spaceship I've been working on, but it was blocked.

I've not started on duration and timepoint, so feel free to implement them.

If you want to look at other things for the spaceship operator. I *think* not all items of paper P1614 are on the list, specifically the removal of operator!=, which will be automatically generated.
If you want to update the status page with these missing entries it would be very much appreciated.

Thank you for this suggestion and the review! Sure, I'll look into P1614R2 and see what I can do when I have time and I'll report back on Discord if necessary.

libcxx/test/std/time/time.duration/time.duration.comparisons/compare.three_way.pass.cpp

66

I hope I resolved it as you requested.

[Mordante]

Thanks! LGTM! IIRC you have commit access right?

[H-G-Hristov]

In D145881#4204872, @Mordante wrote:

Thanks! LGTM! IIRC you have commit access right?

Thank you for the review!

I should have commit access now but it will be my first attempt.

I've setup SSH keys with my GitHub account.
Would that be the right procedure:

$ cd llvm-project
$ git checkout P1614R2-spaceship-operator-duration
$ arc land

This page describes a git based approach https://llvm.org/docs/Contributing.html

The CI failures appear unrelated. Shall I try to commit now?

[Mordante]

In D145881#4207179, @H-G-Hristov wrote:

In D145881#4204872, @Mordante wrote:

Thanks! LGTM! IIRC you have commit access right?

Thank you for the review!

I should have commit access now but it will be my first attempt.

I've setup SSH keys with my GitHub account.
Would that be the right procedure:

$ cd llvm-project
$ git checkout P1614R2-spaceship-operator-duration
$ arc land

This page describes a git based approach https://llvm.org/docs/Contributing.html

The CI failures appear unrelated. Shall I try to commit now?

If you created the review you can indeed use arc land. Feel free to commit the change!

If you run into issues best visit Discord, there we're faster to respond.

[H-G-Hristov]

Rebased

[H-G-Hristov]

Rebased

[Mordante]

In D145881#4210457, @H-G-Hristov wrote:

Rebased

Nice I see committing work!

FYI arc land will automatically rebase for you. There is no need to update the review with these changes.

[H-G-Hristov]

In D145881#4210481, @Mordante wrote:

In D145881#4210457, @H-G-Hristov wrote:

Rebased

Nice I see committing work!

FYI arc land will automatically rebase for you. There is no need to update the review with these changes.

Thank you very much!

I got two error messages one after another. So I thought that I needed to rebase to fix them:

To GitHub-Zingam:llvm/llvm-project.git
! [rejected] cac4b7737e82 -> main (fetch first)
error: failed to push some refs to 'GitHub-Zingam:llvm/llvm-project.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

LOAD STATE Restoring local state (to ref "hgh/libcxx/P1614R2-spaceship-operator-duration" at commit "1d649b88b388").
USAGE EXCEPTION Push failed! Fix the error and run "arc land" again.

And then:

INTO COMMIT Preparing merge into "main" from remote "origin", at commit "fa6ea7a419f3".

<!> UNKNOWN REVISION
Unable to determine which revision is associated with commit "934d897e41a9".
Use "arc diff" to create or update a revision with this commit, or
"--revision" to force selection of a particular revision.

USAGE EXCEPTION Unable to determine revision for commit "934d897e41a9".

Only after the second rebase and arc diff --update-ing twice the arc land completed successfuly.

Maybe I did something wrong. I hope that it's not a problem.

[Mordante]

In D145881#4210510, @H-G-Hristov wrote:

In D145881#4210481, @Mordante wrote:

In D145881#4210457, @H-G-Hristov wrote:

Rebased

Nice I see committing work!

FYI arc land will automatically rebase for you. There is no need to update the review with these changes.

Thank you very much!

I got two error messages one after another. So I thought that I needed to rebase to fix them:

<snip>

Only after the second rebase and arc diff --update-ing twice the arc land completed successfuly.

Maybe I did something wrong. I hope that it's not a problem.

This happens sometimes, the arc land operation is not atomic. For example

• you fetch the commits
• somebody else lands their patch
• you rebase
• now your push fails

I normally just do arc land a second time, and occasionally a third time.

[H-G-Hristov]

In D145881#4210553, @Mordante wrote:

This happens sometimes, the arc land operation is not atomic. For example

• you fetch the commits
• somebody else lands their patch
• you rebase
• now your push fails

I normally just do arc land a second time, and occasionally a third time.

Thanks!

libcxx/docs/Status/SpaceshipProjects.csv

 | `[queue.ops] <https://wg21.link/queue.ops>`_,| queue,None,Unassigned,|Not Started|
 | `[stack.ops] <https://wg21.link/stack.ops>`_,| stack,None,Unassigned,|Not Started|
 | `[reverse.iter.cmp] <https://wg21.link/reverse.iter.cmp>`_,| `reverse_iterator <https://reviews.llvm.org/D113695>`_,None,Mikhail Maltsev,|Complete|
 | `[move.iter.op.comp] <https://wg21.link/move.iter.op.comp>`_,| `move_iterator <https://reviews.llvm.org/D117656>`_,None,Arthur O'Dwyer,|Complete|
 | `[counted.iter.cmp] <https://wg21.link/counted.iter.cmp>`_,| `counted_iterator <https://reviews.llvm.org/D106205>`_,None,Zoe Carver,|Complete|
 | `[range.iota.iterator] <https://wg21.link/range.iota.iterator>`_,| `ranges::iota_view::iterator <https://reviews.llvm.org/D110774>`_,[concepts.cmp],Arthur O'Dwyer,|Complete|
 | `[range.transform.iterator] <https://wg21.link/range.transform.iterator>`_,| `ranges::transform_view::iterator <https://reviews.llvm.org/D110774>`_,[concepts.cmp],Arthur O'Dwyer,|Complete|
 | `[range.elements.iterator] <https://wg21.link/range.elements.iterator>`_,| ranges::elements_view::iterator,[concepts.cmp],Hui Xie,|Complete|
-| `[time.duration.comparisons] <https://wg21.link/time.duration.comparisons>`_, "chrono::duration", None, Mark de Wever, |Not Started|
+| `[time.duration.comparisons] <https://wg21.link/time.duration.comparisons>`_, `chrono::duration <https://reviews.llvm.org/D145881>`_, None, Hristo Hristov, |Complete|
 | `[time.point.comparisons] <https://wg21.link/time.point.comparisons>`_, "chrono::time_point", None, Mark de Wever, |Not Started|
 "| `[time.cal.day.nonmembers] <https://wg21.link/time.cal.day.nonmembers>`_
 | `[time.cal.month.nonmembers] <https://wg21.link/time.cal.month.nonmembers>`_
 | `[time.cal.year.nonmembers] <https://wg21.link/time.cal.year.nonmembers>`_
 | `[time.cal.md.nonmembers] <https://wg21.link/time.cal.md.nonmembers>`_
 | `[time.cal.mdlast] <https://wg21.link/time.cal.mdlast>`_
 | `[time.cal.ym.nonmembers] <https://wg21.link/time.cal.ym.nonmembers>`_
 | `[time.cal.ymd.nonmembers] <https://wg21.link/time.cal.ymd.nonmembers>`_

libcxx/include/__chrono/duration.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___CHRONO_DURATION_H
 #define _LIBCPP___CHRONO_DURATION_H
 
+#include <__compare/ordering.h>
+#include <__compare/three_way_comparable.h>
 #include <__config>
 #include <__type_traits/common_type.h>
 #include <__type_traits/enable_if.h>
 #include <__type_traits/is_convertible.h>
 #include <__type_traits/is_floating_point.h>
 #include <limits>
 #include <ratio>
 

 inline _LIBCPP_INLINE_VISIBILITY
 _LIBCPP_CONSTEXPR
 bool
 operator==(const duration<_Rep1, _Period1>& __lhs, const duration<_Rep2, _Period2>& __rhs)
 {
     return __duration_eq<duration<_Rep1, _Period1>, duration<_Rep2, _Period2> >()(__lhs, __rhs);
 }
 
+#if _LIBCPP_STD_VER <= 17
+
 // Duration !=
 
 template <class _Rep1, class _Period1, class _Rep2, class _Period2>
 inline _LIBCPP_INLINE_VISIBILITY
 _LIBCPP_CONSTEXPR
 bool
 operator!=(const duration<_Rep1, _Period1>& __lhs, const duration<_Rep2, _Period2>& __rhs)
 {
     return !(__lhs == __rhs);
 }
 
+#endif // _LIBCPP_STD_VER <= 17
+
 // Duration <
 
 template <class _LhsDuration, class _RhsDuration>
 struct __duration_lt
 {
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
     bool operator()(const _LhsDuration& __lhs, const _RhsDuration& __rhs) const
         {

 inline _LIBCPP_INLINE_VISIBILITY
 _LIBCPP_CONSTEXPR
 bool
 operator>=(const duration<_Rep1, _Period1>& __lhs, const duration<_Rep2, _Period2>& __rhs)
 {
     return !(__lhs < __rhs);
 }
 
+#if _LIBCPP_STD_VER >= 20
+
+template<class _Rep1, class _Period1, class _Rep2, class _Period2>
+  requires three_way_comparable<common_type_t<_Rep1, _Rep2>>
+_LIBCPP_HIDE_FROM_ABI
+constexpr auto operator<=>(const duration<_Rep1, _Period1>& __lhs,
+                           const duration<_Rep2, _Period2>& __rhs)
+{
+    using _Ct = common_type_t<duration<_Rep1, _Period1>, duration<_Rep2, _Period2>>;
+    return _Ct(__lhs).count() <=> _Ct(__rhs).count();
+}
+
+#endif // _LIBCPP_STD_VER >= 20
+
 // Duration +
 
 template <class _Rep1, class _Period1, class _Rep2, class _Period2>
 inline _LIBCPP_INLINE_VISIBILITY
 _LIBCPP_CONSTEXPR
 typename common_type<duration<_Rep1, _Period1>, duration<_Rep2, _Period2> >::type
 operator+(const duration<_Rep1, _Period1>& __lhs, const duration<_Rep2, _Period2>& __rhs)
 {

libcxx/include/chrono

template <class Rep1, class Period1, class Rep2, class Period2>
   operator/(const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
 
 // duration comparisons
 template <class Rep1, class Period1, class Rep2, class Period2>
    constexpr
    bool operator==(const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
 template <class Rep1, class Period1, class Rep2, class Period2>
    constexpr
-   bool operator!=(const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
+   bool operator!=(const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs); // removed in C++20
 template <class Rep1, class Period1, class Rep2, class Period2>
    constexpr
    bool operator< (const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
 template <class Rep1, class Period1, class Rep2, class Period2>
    constexpr
    bool operator<=(const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
 template <class Rep1, class Period1, class Rep2, class Period2>
    constexpr
    bool operator> (const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
 template <class Rep1, class Period1, class Rep2, class Period2>
    constexpr
    bool operator>=(const duration<Rep1, Period1>& lhs, const duration<Rep2, Period2>& rhs);
+template<class Rep1, class Period1, class Rep2, class Period2>
+  requires three_way_comparable<typename CT::rep>
+  constexpr auto operator<=>(const duration<Rep1, Period1>& lhs,
+                             const duration<Rep2, Period2>& rhs);                           // since C++20
 
 // duration_cast
 template <class ToDuration, class Rep, class Period>
   ToDuration duration_cast(const duration<Rep, Period>& d);
 
 template <class ToDuration, class Rep, class Period>
     constexpr ToDuration floor(const duration<Rep, Period>& d);    // C++17
 template <class ToDuration, class Rep, class Period>

libcxx/test/std/time/time.duration/time.duration.comparisons/compare.three_way.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// <chrono>
+
+// duration
+
+// template<class Rep1, class Period1, class Rep2, class Period2>
+//     requires ThreeWayComparable<typename CT::rep>
+//   constexpr auto operator<=>(const duration<Rep1, Period1>& lhs,
+//                              const duration<Rep2, Period2>& rhs);
+
+#include <cassert>
+#include <chrono>
+#include <ratio>
+
+#include "test_comparisons.h"
+
+constexpr bool test() {
+  {
+    std::chrono::seconds s1(3);
+    std::chrono::seconds s2(3);
+    assert((s1 <=> s2) == std::strong_ordering::equal);
+    assert(testOrder(s1, s2, std::strong_ordering::equal));
+  }
+  {
+    std::chrono::seconds s1(3);
+    std::chrono::seconds s2(4);
+    assert((s1 <=> s2) == std::strong_ordering::less);
+    assert(testOrder(s1, s2, std::strong_ordering::less));
+  }
+  {
+    std::chrono::milliseconds s1(3);
+    std::chrono::microseconds s2(3000);
+    assert((s1 <=> s2) == std::strong_ordering::equal);
+    assert(testOrder(s1, s2, std::strong_ordering::equal));
+  }
+  {
+    std::chrono::milliseconds s1(3);
+    std::chrono::microseconds s2(4000);
+    assert((s1 <=> s2) == std::strong_ordering::less);
+    assert(testOrder(s1, s2, std::strong_ordering::less));
+  }
+  {
+    std::chrono::duration<int, std::ratio<2, 3>> s1(9);
+    std::chrono::duration<int, std::ratio<3, 5>> s2(10);
+    assert((s1 <=> s2) == std::strong_ordering::equal);
+    assert(testOrder(s1, s2, std::strong_ordering::equal));
+  }
+  {
+    std::chrono::duration<int, std::ratio<2, 3>> s1(10);
+    std::chrono::duration<int, std::ratio<3, 5>> s2(9);
+    assert((s1 <=> s2) == std::strong_ordering::greater);
+    assert(testOrder(s1, s2, std::strong_ordering::greater));
+  }
+  {
+    std::chrono::duration<int, std::ratio<2, 3>> s1(9);
+    std::chrono::duration<double, std::ratio<3, 5>> s2(10.1);
+    assert((s1 <=> s2) == std::strong_ordering::less);
+    assert(testOrder(s1, s2, std::strong_ordering::less));
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  assert(test());
+  static_assert(test());
+  return 0;
+}