Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][chrono] Fixes leap seconds. #90070

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

mordante
Copy link
Member

While implementing the UTC clock it turns out that the implementation of the leap seconds was not correct, it should store the individual value, not the sum.

It also looks like LWG3359 has not been fully implemented.

Implements parts of:

  • LWG3359 leap second support should allow for negative leap seconds

@mordante mordante requested a review from a team as a code owner April 25, 2024 15:26
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 25, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 25, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

While implementing the UTC clock it turns out that the implementation of the leap seconds was not correct, it should store the individual value, not the sum.

It also looks like LWG3359 has not been fully implemented.

Implements parts of:

  • LWG3359 <chrono> leap second support should allow for negative leap seconds

Full diff: https://github.com/llvm/llvm-project/pull/90070.diff

4 Files Affected:

  • (modified) libcxx/docs/Status/Cxx20Issues.csv (+1-1)
  • (modified) libcxx/src/tzdb.cpp (+40-24)
  • (modified) libcxx/test/libcxx/time/time.zone/time.zone.db/leap_seconds.pass.cpp (+15-8)
  • (modified) libcxx/test/std/time/time.zone/time.zone.db/leap_seconds.pass.cpp (+27-28)
diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index db57b15256a62f..fed1c7e736248f 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -269,7 +269,7 @@
 "`3355 <https://wg21.link/LWG3355>`__","The memory algorithms should support move-only input iterators introduced by P1207","Prague","|Complete|","15.0","|ranges|"
 "`3356 <https://wg21.link/LWG3356>`__","``__cpp_lib_nothrow_convertible``\  should be ``__cpp_lib_is_nothrow_convertible``\ ","Prague","|Complete|","12.0"
 "`3358 <https://wg21.link/LWG3358>`__","|sect|\ [span.cons] is mistaken that ``to_address``\  can throw","Prague","|Complete|","17.0"
-"`3359 <https://wg21.link/LWG3359>`__","``<chrono>``\  leap second support should allow for negative leap seconds","Prague","|Complete|","19.0","|chrono|"
+"`3359 <https://wg21.link/LWG3359>`__","``<chrono>``\  leap second support should allow for negative leap seconds","Prague","|In Progress|","","|chrono|"
 "`3360 <https://wg21.link/LWG3360>`__","``three_way_comparable_with``\  is inconsistent with similar concepts","Prague","|Nothing To Do|","","|spaceship|"
 "`3362 <https://wg21.link/LWG3362>`__","Strike ``stop_source``\ 's ``operator!=``\ ","Prague","",""
 "`3363 <https://wg21.link/LWG3363>`__","``drop_while_view``\  should opt-out of ``sized_range``\ ","Prague","|Nothing To Do|","","|ranges|"
diff --git a/libcxx/src/tzdb.cpp b/libcxx/src/tzdb.cpp
index 5951e59248e94f..8588646bbbc417 100644
--- a/libcxx/src/tzdb.cpp
+++ b/libcxx/src/tzdb.cpp
@@ -626,29 +626,49 @@ static void __parse_leap_seconds(vector<leap_second>& __leap_seconds, istream&&
   // seconds since 1 January 1970.
   constexpr auto __offset = sys_days{1970y / January / 1} - sys_days{1900y / January / 1};
 
-  while (true) {
-    switch (__input.peek()) {
-    case istream::traits_type::eof():
-      return;
-
-    case ' ':
-    case '\t':
-    case '\n':
-      __input.get();
-      continue;
+  struct __entry {
+    sys_seconds __timestamp;
+    seconds __value;
+  };
+  vector<__entry> __entries;
+  [&] {
+    while (true) {
+      switch (__input.peek()) {
+      case istream::traits_type::eof():
+        return;
+
+      case ' ':
+      case '\t':
+      case '\n':
+        __input.get();
+        continue;
+
+      case '#':
+        chrono::__skip_line(__input);
+        continue;
+      }
 
-    case '#':
+      sys_seconds __date = sys_seconds{seconds{chrono::__parse_integral(__input, false)}} - __offset;
+      chrono::__skip_mandatory_whitespace(__input);
+      seconds __value{chrono::__parse_integral(__input, false)};
       chrono::__skip_line(__input);
-      continue;
-    }
 
-    sys_seconds __date = sys_seconds{seconds{chrono::__parse_integral(__input, false)}} - __offset;
-    chrono::__skip_mandatory_whitespace(__input);
-    seconds __value{chrono::__parse_integral(__input, false)};
-    chrono::__skip_line(__input);
-
-    __leap_seconds.emplace_back(std::__private_constructor_tag{}, __date, __value);
-  }
+      __entries.emplace_back(__date, __value);
+    }
+  }();
+  // 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(__entries, {}, &__entry::__timestamp);
+
+  // The database should contain the number of seconds inserted by a leap
+  // second (1 or -1). So the difference between the two elements are stored.
+  // std::ranges::views::adjacent has not been implemented yet.
+  (void)ranges::adjacent_find(__entries, [&](const __entry& __first, const __entry& __second) {
+    __leap_seconds.emplace_back(
+        std::__private_constructor_tag{}, __second.__timestamp, __second.__value - __first.__value);
+    return false;
+  });
 }
 
 void __init_tzdb(tzdb& __tzdb, __tz::__rules_storage_type& __rules) {
@@ -667,10 +687,6 @@ 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"});
-  // 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);
 }
 
 #ifdef _WIN32
diff --git a/libcxx/test/libcxx/time/time.zone/time.zone.db/leap_seconds.pass.cpp b/libcxx/test/libcxx/time/time.zone/time.zone.db/leap_seconds.pass.cpp
index 25a0f00003da2f..d7ae21926b4b26 100644
--- a/libcxx/test/libcxx/time/time.zone/time.zone.db/leap_seconds.pass.cpp
+++ b/libcxx/test/libcxx/time/time.zone/time.zone.db/leap_seconds.pass.cpp
@@ -83,32 +83,39 @@ static void test_leap_seconds() {
 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
+86400        9  # 2 Jan 1900 Dummy entry to test before 1970
+1            8  # 2 Jan 1900 Dummy entry to test before 1970
+
+# Fictional negative leap second
+2303769600  11  # 2 Jan 1973
 
 # largest accepted value by the parser
-5764607523034234879 2
+5764607523034234879 12
 )");
 
-  assert(result.leap_seconds.size() == 5);
+  assert(result.leap_seconds.size() == 6);
 
   assert(result.leap_seconds[0].date() == sys_seconds{sys_days{1900y / January / 2}});
   assert(result.leap_seconds[0].value() == 1s);
 
   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].value() == 1s);
 
   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].value() == 1s);
 
   assert(result.leap_seconds[3].date() == sys_seconds{sys_days{1973y / January / 1}});
-  assert(result.leap_seconds[3].value() == 12s);
+  assert(result.leap_seconds[3].value() == 1s);
+
+  assert(result.leap_seconds[4].date() == sys_seconds{sys_days{1973y / January / 2}});
+  assert(result.leap_seconds[4].value() == -1s);
 
-  assert(result.leap_seconds[4].date() ==
+  assert(result.leap_seconds[5].date() ==
          sys_seconds{5764607523034234879s
                      // The database uses 1900-01-01 as epoch.
                      - std::chrono::duration_cast<std::chrono::seconds>(
                            sys_days{1970y / January / 1} - sys_days{1900y / January / 1})});
-  assert(result.leap_seconds[4].value() == 2s);
+  assert(result.leap_seconds[5].value() == 1s);
 }
 
 int main(int, const char**) {
diff --git a/libcxx/test/std/time/time.zone/time.zone.db/leap_seconds.pass.cpp b/libcxx/test/std/time/time.zone/time.zone.db/leap_seconds.pass.cpp
index f873ad3167819e..39023c31aecbbf 100644
--- a/libcxx/test/std/time/time.zone/time.zone.db/leap_seconds.pass.cpp
+++ b/libcxx/test/std/time/time.zone/time.zone.db/leap_seconds.pass.cpp
@@ -33,34 +33,33 @@ using namespace std::literals::chrono_literals;
 // At the moment of writing that list is the actual list in the IANA database.
 // If in the future more leap seconds can be added.
 static const std::array leap_seconds = {
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1972y / std::chrono::January / 1}}, 10s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1972y / std::chrono::July / 1}}, 11s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1973y / std::chrono::January / 1}}, 12s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1974y / std::chrono::January / 1}}, 13s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1975y / std::chrono::January / 1}}, 14s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1976y / std::chrono::January / 1}}, 15s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1977y / std::chrono::January / 1}}, 16s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1978y / std::chrono::January / 1}}, 17s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1979y / std::chrono::January / 1}}, 18s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1980y / std::chrono::January / 1}}, 19s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1981y / std::chrono::July / 1}}, 20s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1982y / std::chrono::July / 1}}, 21s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1983y / std::chrono::July / 1}}, 22s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1985y / std::chrono::July / 1}}, 23s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1988y / std::chrono::January / 1}}, 24s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1990y / std::chrono::January / 1}}, 25s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1991y / std::chrono::January / 1}}, 26s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1992y / std::chrono::July / 1}}, 27s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1993y / std::chrono::July / 1}}, 28s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1994y / std::chrono::July / 1}}, 29s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1996y / std::chrono::January / 1}}, 30s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1997y / std::chrono::July / 1}}, 31s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1999y / std::chrono::January / 1}}, 32s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2006y / std::chrono::January / 1}}, 33s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2009y / std::chrono::January / 1}}, 34s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2012y / std::chrono::July / 1}}, 35s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2015y / std::chrono::July / 1}}, 36s),
-    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2017y / std::chrono::January / 1}}, 37s)};
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1972y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1973y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1974y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1975y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1976y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1977y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1978y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1979y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1980y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1981y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1982y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1983y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1985y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1988y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1990y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1991y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1992y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1993y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1994y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1996y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1997y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{1999y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2006y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2009y / std::chrono::January / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2012y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2015y / std::chrono::July / 1}}, 1s),
+    std::make_pair(std::chrono::sys_seconds{std::chrono::sys_days{2017y / std::chrono::January / 1}}, 1s)};
 
 int main(int, const char**) {
   const std::chrono::tzdb& tzdb = std::chrono::get_tzdb();

@ldionne ldionne self-assigned this Apr 26, 2024
seconds __value;
};
vector<__entry> __entries;
[&] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of wrapping the loop in an IILE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the while loop there is a switch that should terminate the processing when EOF is found. Since test is in a switch I can't use break. Using a IILE allows to use return. Refactoring this lambda to a function doesn't improve code readability, so I prefer an IILE.

libcxx/src/tzdb.cpp Outdated Show resolved Hide resolved
libcxx/src/tzdb.cpp Outdated Show resolved Hide resolved
@mordante mordante force-pushed the users/mordante/get_info_local_time branch from 6939905 to a6b280e Compare June 9, 2024 12:26
Base automatically changed from users/mordante/get_info_local_time to main June 9, 2024 16:21
While implementing the UTC clock it turns out that the implementation of
the leap seconds was not correct, it should store the individual value,
not the sum.

It also looks like LWG3359 has not been fully implemented.

Implements parts of:
- LWG3359 <chrono> leap second support should allow for negative leap seconds
@mordante mordante force-pushed the users/mordante/fixes_leap_second_database_format branch from d094cde to 9e551c1 Compare July 4, 2024 14:38
@mordante mordante merged commit 093ddac into main Jul 4, 2024
58 checks passed
@mordante mordante deleted the users/mordante/fixes_leap_second_database_format branch July 4, 2024 18:45
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
While implementing the UTC clock it turns out that the implementation of
the leap seconds was not correct, it should store the individual value,
not the sum.

It also looks like LWG3359 has not been fully implemented.

Implements parts of:
- LWG3359 <chrono> leap second support should allow for negative leap
seconds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants