Addresses review comments.

Author: mordante

libcxx/include/__chrono/leap_second.h

@@ -52,10 +52,10 @@ class leap_second {
   seconds __value_;
 };
 
-_LIBCPP_HIDE_FROM_ABI constexpr bool operator==(const leap_second& __x, const leap_second& __y) {
+_LIBCPP_HIDE_FROM_ABI inline constexpr bool operator==(const leap_second& __x, const leap_second& __y) {
   return __x.date() == __y.date();
 }
-_LIBCPP_HIDE_FROM_ABI constexpr strong_ordering operator<=>(const leap_second& __x, const leap_second& __y) {
+_LIBCPP_HIDE_FROM_ABI inline constexpr strong_ordering operator<=>(const leap_second& __x, const leap_second& __y) {
   return __x.date() <=> __y.date();
 }
 

libcxx/src/tzdb.cpp

@@ -669,8 +669,9 @@ void __init_tzdb(tzdb& __tzdb, __tz::__rules_storage_type& __rules) {
   // The latter is much easier to parse, it seems Howard shares that
   // opinion.
   chrono::__parse_leap_seconds(__tzdb.leap_seconds, ifstream{__root / "leap-seconds.list"});
-  // Note the input is sorted, but that does not seem to be are
-  // requirement, it is a requirement in the Standard.
+  // The Standard requires the leap seconds to be sorted. The file
+  // leap-seconds.list usually provides them in sorted order, but that is not
+  // guaranteed so we ensure it here.
   std::ranges::sort(__tzdb.leap_seconds);
 }
 

libcxx/test/libcxx/time/time.zone/time.zone.db/leap_seconds.pass.cpp

@@ -75,23 +75,28 @@ static void test_invalid() {
 static void test_leap_seconds() {
   using namespace std::chrono;
 
+  // Test whether loading also sorts the entries in the proper order.
   const tzdb& result = parse(
       R"(
-2272060800  10  # 1 Jan 1972
-2287785600  11  # 1 Jul 1972
 2303683200  12  # 1 Jan 1973
+2287785600  11  # 1 Jul 1972
+2272060800  10  # 1 Jan 1972
+86400        1  # 2 Jan 1900 Dummy entry to test before 1970
 )");
 
-  assert(result.leap_seconds.size() == 3);
+  assert(result.leap_seconds.size() == 4);
+
+  assert(result.leap_seconds[0].date() == sys_seconds{sys_days{1900y / January / 2}});
+  assert(result.leap_seconds[0].value() == 1s);
 
-  assert(result.leap_seconds[0].date() == sys_seconds{sys_days{1972y / January / 1}});
-  assert(result.leap_seconds[0].value() == 10s);
+  assert(result.leap_seconds[1].date() == sys_seconds{sys_days{1972y / January / 1}});
+  assert(result.leap_seconds[1].value() == 10s);
 
-  assert(result.leap_seconds[1].date() == sys_seconds{sys_days{1972y / July / 1}});
-  assert(result.leap_seconds[1].value() == 11s);
+  assert(result.leap_seconds[2].date() == sys_seconds{sys_days{1972y / July / 1}});
+  assert(result.leap_seconds[2].value() == 11s);
 
-  assert(result.leap_seconds[2].date() == sys_seconds{sys_days{1973y / January / 1}});
-  assert(result.leap_seconds[2].value() == 12s);
+  assert(result.leap_seconds[3].date() == sys_seconds{sys_days{1973y / January / 1}});
+  assert(result.leap_seconds[3].value() == 12s);
 }
 
 int main(int, const char**) {

libcxx/test/std/time/time.zone/time.zone.db/time.zone.db.tzdb/tzdb.members.pass.cpp

@@ -37,11 +37,9 @@ int main(int, const char**) {
   tzdb.version = "version";
   assert(tzdb.version == "version");
 
-  [[maybe_unused]] std::vector<std::chrono::time_zone>& zones = tzdb.zones;
-
+  [[maybe_unused]] std::vector<std::chrono::time_zone>& zones          = tzdb.zones;
   [[maybe_unused]] std::vector<std::chrono::time_zone_link>& links = tzdb.links;
-
-  // TODO TZDB add the leap data member
+  [[maybe_unused]] std::vector<std::chrono::leap_second>& leap_seconds = tzdb.leap_seconds;
 
   return 0;
 }

libcxx/test/std/time/time.zone/time.zone.leap/assign.copy.pass.cpp

@@ -22,6 +22,7 @@
 // };
 
 #include <chrono>
+#include <memory>
 #include <type_traits>
 #include <cassert>
 
@@ -39,7 +40,9 @@ constexpr bool test() {
   assert(a.date() != b.date());
   assert(a.value() != b.value());
 
-  b = a;
+  std::same_as<std::chrono::leap_second&> decltype(auto) result(b = a);
+  assert(std::addressof(result) == std::addressof(b));
+
   assert(a.date() == b.date());
   assert(a.value() == b.value());
 

libcxx/test/std/time/time.zone/time.zone.leap/cons.copy.pass.cpp

@@ -32,7 +32,7 @@
 constexpr bool test() {
   std::chrono::leap_second a =
       test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1});
-  std::chrono::leap_second b(a);
+  std::chrono::leap_second b = a;
 
   //  operator== only compares the date member.
   assert(a.date() == b.date());

libcxx/test/std/time/time.zone/time.zone.leap/members/date.pass.cpp

@@ -27,22 +27,27 @@
 // ADDITIONAL_COMPILE_FLAGS(stdlib=libc++): -I %S/../../../../../../src/include
 #include "test_chrono_leap_second.h"
 
-constexpr bool test(const std::chrono::leap_second leap_second, std::chrono::sys_seconds expected) {
+constexpr void test(const std::chrono::leap_second leap_second, std::chrono::sys_seconds expected) {
   std::same_as<std::chrono::sys_seconds> auto date = leap_second.date();
   assert(date == expected);
   static_assert(noexcept(leap_second.date()));
+}
+
+constexpr bool test() {
+  test(test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1}),
+       std::chrono::sys_seconds{std::chrono::seconds{0}});
 
   return true;
 }
 
 int main(int, const char**) {
+  test();
+  static_assert(test());
+
+  // test with the real tzdb
   const std::chrono::tzdb& tzdb = std::chrono::get_tzdb();
   assert(!tzdb.leap_seconds.empty());
   test(tzdb.leap_seconds[0], tzdb.leap_seconds[0].date());
 
-  static_assert(
-      test(test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1}),
-           std::chrono::sys_seconds{std::chrono::seconds{0}}));
-
   return 0;
 }

libcxx/test/std/time/time.zone/time.zone.leap/members/value.pass.cpp

@@ -27,22 +27,27 @@
 // ADDITIONAL_COMPILE_FLAGS(stdlib=libc++): -I %S/../../../../../../src/include
 #include "test_chrono_leap_second.h"
 
-constexpr bool test(const std::chrono::leap_second leap_second, std::chrono::seconds expected) {
+constexpr void test(const std::chrono::leap_second leap_second, std::chrono::seconds expected) {
   std::same_as<std::chrono::seconds> auto value = leap_second.value();
   assert(value == expected);
   static_assert(noexcept(leap_second.value()));
+}
+
+constexpr bool test() {
+  test(test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1}),
+       std::chrono::seconds{1});
 
   return true;
 }
 
 int main(int, const char**) {
+  test();
+  static_assert(test());
+
+  // test with the real tzdb
   const std::chrono::tzdb& tzdb = std::chrono::get_tzdb();
   assert(!tzdb.leap_seconds.empty());
   test(tzdb.leap_seconds[0], tzdb.leap_seconds[0].value());
 
-  static_assert(
-      test(test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1}),
-           std::chrono::seconds{1}));
-
   return 0;
 }

libcxx/test/std/time/time.zone/time.zone.leap/nonmembers/comparison.pass.cpp

@@ -51,7 +51,7 @@
 // ADDITIONAL_COMPILE_FLAGS(stdlib=libc++): -I %S/../../../../../../src/include
 #include "test_chrono_leap_second.h"
 
-constexpr bool test(const std::chrono::leap_second lhs, const std::chrono::leap_second rhs) {
+constexpr void test(const std::chrono::leap_second lhs, const std::chrono::leap_second rhs) {
   AssertOrderReturn<std::strong_ordering, std::chrono::leap_second>();
   assert(testOrder(lhs, rhs, std::strong_ordering::less));
 
@@ -60,18 +60,23 @@ constexpr bool test(const std::chrono::leap_second lhs, const std::chrono::leap_
 
   AssertOrderReturn<std::strong_ordering, std::chrono::sys_seconds, std::chrono::leap_second>();
   assert(testOrder(lhs.date(), rhs, std::strong_ordering::less));
+}
+
+constexpr bool test() {
+  test(test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1}),
+       test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{1}}, std::chrono::seconds{2}));
 
   return true;
 }
 
 int main(int, const char**) {
+  test();
+  static_assert(test());
+
+  // test with the real tzdb
   const std::chrono::tzdb& tzdb = std::chrono::get_tzdb();
   assert(tzdb.leap_seconds.size() > 2);
   test(tzdb.leap_seconds[0], tzdb.leap_seconds[1]);
 
-  static_assert(
-      test(test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{0}}, std::chrono::seconds{1}),
-           test_leap_second_create(std::chrono::sys_seconds{std::chrono::seconds{1}}, std::chrono::seconds{2})));
-
   return 0;
 }

libcxx/test/support/test_chrono_leap_second.h

@@ -10,19 +10,17 @@
 #ifndef SUPPORT_TEST_CHRONO_LEAP_SECOND_HPP
 #define SUPPORT_TEST_CHRONO_LEAP_SECOND_HPP
 
-/**
- * @file Helper functions to create a @ref std::chrono::leap_second.
- *
- * Since the standard doesn't specify how a @ref std::chrono::leap_second is
- * constructed this is implementation defined. To make the public API tests of
- * the class generic this header defines helper functions to create the
- * required object.
- *
- * @note This requires every standard library implementation to write their own
- * helper function. Vendors are encouraged to create a pull request at
- * https://github.com/llvm/llvm-project so their specific implementation can be
- * part of this file.
- */
+// Contains helper functions to create a std::chrono::leap_second.
+//
+// Since the standard doesn't specify how a @ref std::chrono::leap_second is
+// constructed this is implementation defined. To make the public API tests of
+// the class generic this header defines helper functions to create the
+// required object.
+//
+// Note This requires every standard library implementation to write their own
+// helper function. Vendors are encouraged to create a pull request at
+// https://github.com/llvm/llvm-project so their specific implementation can be
+// part of this file.
 
 #include "test_macros.h"
 
@@ -36,7 +34,7 @@
 
 // In order to find this include the calling test needs to provide this path in
 // the search path. Typically this looks like:
-//   REMOVE_THIS_PREFIX__ADDITIONAL_COMPILE_FLAGS(stdlib=libc++): -I %S/../../../../../../src/include
+//   ADDITIONAL_COMPILE_FLAGS(stdlib=libc++): -I %S/../../../../../../src/include
 // where the number of `../` sequences depends on the subdirectory level of the
 // test.
 #  include "tzdb/leap_second_private.h" // Header in the dylib
@@ -47,7 +45,8 @@ test_leap_second_create(const std::chrono::sys_seconds& date, const std::chrono:
 }
 
 #else // _LIBCPP_VERSION
-#  error "Please create a vendor specific version of the test functions and file a review at https://reviews.llvm.org/"
+#  error                                                                                                               \
+      "Please create a vendor specific version of the test typedef and file a PR at https://github.com/llvm/llvm-project"
 #endif // _LIBCPP_VERSION
 
 #endif // SUPPORT_TEST_CHRONO_LEAP_SECOND_HPP