[libc++][chrono] Adds the sys_info class. #85619
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Mark de Wever (mordante) ChangesAdds the sys_info class and time_zone::get_info(). The code still has a few quirks and has not been optimized for performance yet. The returned sys_info is compared against the output of the zdump tool in the test giving confidence the implementation is correct. Implements parts of:
Implements:
Patch is 119.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85619.diff 17 Files Affected:
diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index 58e995809777c1..a6aa70ecc866e0 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -41,4 +41,5 @@
"`4001 <https://wg21.link/LWG4001>`__","``iota_view`` should provide ``empty``","Kona November 2023","","","|ranges|"
"","","","","",""
"`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""
+"XXXX","","The sys_info range should be affected by save","Not Yet Adopted","|Complete|","19.0"
"","","","","",""
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 5d9a693d152e4a..24cef9cd877c62 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -289,6 +289,7 @@ set(files
__chrono/ostream.h
__chrono/parser_std_format_spec.h
__chrono/statically_widen.h
+ __chrono/sys_info.h
__chrono/steady_clock.h
__chrono/system_clock.h
__chrono/time_point.h
diff --git a/libcxx/include/__chrono/sys_info.h b/libcxx/include/__chrono/sys_info.h
new file mode 100644
index 00000000000000..16ec5dd254d59a
--- /dev/null
+++ b/libcxx/include/__chrono/sys_info.h
@@ -0,0 +1,53 @@
+// -*- 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
+//
+//===----------------------------------------------------------------------===//
+
+// For information see https://libcxx.llvm.org/DesignDocs/TimeZone.html
+
+#ifndef _LIBCPP___CHRONO_SYS_INFO_H
+#define _LIBCPP___CHRONO_SYS_INFO_H
+
+#include <version>
+// Enable the contents of the header only when libc++ was built with experimental features enabled.
+#if !defined(_LIBCPP_HAS_NO_INCOMPLETE_TZDB)
+
+# include <__chrono/duration.h>
+# include <__chrono/system_clock.h>
+# include <__chrono/time_point.h>
+# include <__config>
+# include <string>
+
+# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+# endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+# if _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
+ !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
+namespace chrono {
+
+struct sys_info {
+ sys_seconds begin;
+ sys_seconds end;
+ seconds offset;
+ minutes save;
+ string abbrev;
+};
+
+} // namespace chrono
+
+# endif // _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM)
+ // && !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // !defined(_LIBCPP_HAS_NO_INCOMPLETE_TZDB)
+
+#endif // _LIBCPP___CHRONO_SYS_INFO_H
diff --git a/libcxx/include/__chrono/time_zone.h b/libcxx/include/__chrono/time_zone.h
index 7d97327a6c8e99..a5af6297a4cf92 100644
--- a/libcxx/include/__chrono/time_zone.h
+++ b/libcxx/include/__chrono/time_zone.h
@@ -16,6 +16,7 @@
// Enable the contents of the header only when libc++ was built with experimental features enabled.
#if !defined(_LIBCPP_HAS_NO_INCOMPLETE_TZDB)
+# include <__chrono/sys_info.h>
# include <__compare/strong_order.h>
# include <__config>
# include <__memory/unique_ptr.h>
@@ -55,10 +56,18 @@ class _LIBCPP_AVAILABILITY_TZDB time_zone {
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI string_view name() const noexcept { return __name(); }
+ template <class _Duration>
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI sys_info get_info(const sys_time<_Duration>& __time) const {
+ return __get_info(chrono::time_point_cast<seconds>(__time));
+ }
+
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI const __impl& __implementation() const noexcept { return *__impl_; }
private:
[[nodiscard]] _LIBCPP_EXPORTED_FROM_ABI string_view __name() const noexcept;
+
+ [[nodiscard]] _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI sys_info __get_info(sys_seconds __time) const;
+
unique_ptr<__impl> __impl_;
};
diff --git a/libcxx/include/chrono b/libcxx/include/chrono
index 8fdc30a3624dfc..00b940a6610a3a 100644
--- a/libcxx/include/chrono
+++ b/libcxx/include/chrono
@@ -724,6 +724,15 @@ const time_zone* current_zone()
const tzdb& reload_tzdb(); // C++20
string remote_version(); // C++20
+// [time.zone.info], information classes
+struct sys_info { // C++20
+ sys_seconds begin;
+ sys_seconds end;
+ seconds offset;
+ minutes save;
+ string abbrev;
+};
+
// 25.10.5, class time_zone // C++20
enum class choose {earliest, latest};
class time_zone {
@@ -733,6 +742,9 @@ class time_zone {
// unspecified additional constructors
string_view name() const noexcept;
+
+ template<class Duration>
+ sys_info get_info(const sys_time<Duration>& st) const;
};
bool operator==(const time_zone& x, const time_zone& y) noexcept; // C++20
strong_ordering operator<=>(const time_zone& x, const time_zone& y) noexcept; // C++20
@@ -906,6 +918,7 @@ constexpr chrono::year operator ""y(unsigned lo
#if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
!defined(_LIBCPP_HAS_NO_LOCALIZATION)
# include <__chrono/leap_second.h>
+# include <__chrono/sys_info.h>
# include <__chrono/time_zone.h>
# include <__chrono/time_zone_link.h>
# include <__chrono/tzdb.h>
diff --git a/libcxx/include/libcxx.imp b/libcxx/include/libcxx.imp
index 2d6ac5f9e982aa..e3a08860c0b059 100644
--- a/libcxx/include/libcxx.imp
+++ b/libcxx/include/libcxx.imp
@@ -287,6 +287,7 @@
{ include: [ "<__chrono/parser_std_format_spec.h>", "private", "<chrono>", "public" ] },
{ include: [ "<__chrono/statically_widen.h>", "private", "<chrono>", "public" ] },
{ include: [ "<__chrono/steady_clock.h>", "private", "<chrono>", "public" ] },
+ { include: [ "<__chrono/sys_info.h>", "private", "<chrono>", "public" ] },
{ include: [ "<__chrono/system_clock.h>", "private", "<chrono>", "public" ] },
{ include: [ "<__chrono/time_point.h>", "private", "<chrono>", "public" ] },
{ include: [ "<__chrono/time_zone.h>", "private", "<chrono>", "public" ] },
diff --git a/libcxx/modules/std/chrono.inc b/libcxx/modules/std/chrono.inc
index e14228043d3b84..575e6347aecce1 100644
--- a/libcxx/modules/std/chrono.inc
+++ b/libcxx/modules/std/chrono.inc
@@ -212,10 +212,12 @@ export namespace std {
// [time.zone.exception], exception classes
using std::chrono::ambiguous_local_time;
using std::chrono::nonexistent_local_time;
+# endif // if 0
// [time.zone.info], information classes
using std::chrono::sys_info;
+# if 0
// [time.zone.timezone], class time_zone
using std::chrono::choose;
# endif // if 0
diff --git a/libcxx/src/include/tzdb/types_private.h b/libcxx/src/include/tzdb/types_private.h
index 4604b9fc88114d..bdc9418a8866be 100644
--- a/libcxx/src/include/tzdb/types_private.h
+++ b/libcxx/src/include/tzdb/types_private.h
@@ -33,7 +33,17 @@ namespace chrono::__tz {
// Sun>=8 first Sunday on or after the eighth
// Sun<=25 last Sunday on or before the 25th
struct __constrained_weekday {
- /* year_month_day operator()(year __year, month __month);*/ // needed but not implemented
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI year_month_day operator()(year __year, month __month) const {
+ auto __result = static_cast<sys_days>(year_month_day{__year, __month, __day});
+ weekday __wd{static_cast<sys_days>(__result)};
+
+ if (__comparison == __le)
+ __result -= __wd - __weekday;
+ else
+ __result += __weekday - __wd;
+
+ return __result;
+ }
weekday __weekday;
enum __comparison_t { __le, __ge } __comparison;
@@ -85,7 +95,8 @@ struct __continuation {
// used.
// If this field contains - then standard time always
// applies. This is indicated by the monostate.
- using __rules_t = variant<monostate, __tz::__save, string, size_t>;
+ // TODO TZDB Investigate implemention the size_t based caching.
+ using __rules_t = variant<monostate, __tz::__save, string /*, size_t*/>;
__rules_t __rules;
diff --git a/libcxx/src/include/tzdb/tzdb_list_private.h b/libcxx/src/include/tzdb/tzdb_list_private.h
index f43d7d8ea772be..a3f56e79a51791 100644
--- a/libcxx/src/include/tzdb/tzdb_list_private.h
+++ b/libcxx/src/include/tzdb/tzdb_list_private.h
@@ -84,6 +84,13 @@ class tzdb_list::__impl {
const_iterator cbegin() const noexcept { return begin(); }
const_iterator cend() const noexcept { return end(); }
+ const __tz::__rules_storage_type& __rules() const noexcept {
+#ifndef _LIBCPP_HAS_NO_THREADS
+ shared_lock __lock{__mutex_};
+#endif
+ return __rules_.front();
+ }
+
private:
// Loads the tzdbs
// pre: The caller ensures the locking, if needed, is done.
diff --git a/libcxx/src/time_zone.cpp b/libcxx/src/time_zone.cpp
index b6bf06a116f68b..84ff0e1ae49268 100644
--- a/libcxx/src/time_zone.cpp
+++ b/libcxx/src/time_zone.cpp
@@ -8,14 +8,706 @@
// For information see https://libcxx.llvm.org/DesignDocs/TimeZone.html
+// TODO TZDB look at optimizations
+//
+// The current algorithm is correct but not efficient. For example, in a named
+// rule based continuation finding the next rule does quite a bit of work,
+// returns the next rule and "forgets" its state. This could be better.
+//
+// It would be possible to cache lookups. If a time for a zone is calculated its
+// sys_info could be kept and the next lookup could test whether the time is in
+// a "known" sys_info. The wording in the Standard hints at this slowness by
+// "suggesting" this could be implemented at the user's side.
+
+// TODO TZDB look at removing quirks
+//
+// The code has some special rules to adjust the timing at the continuation
+// switches. This works correctly, but some of the places feel odd. It would be
+// good to investigate this further and see whether all quirks are needed or
+// that there are better fixes.
+//
+// These quirks often use a 12h interval; this is the scan interval of zdump,
+// which implies there are no sys_info objects with a duration of less than 12h.
+
+#include <algorithm>
#include <chrono>
+#include <expected>
+#include <map>
+#include <ranges>
#include "include/tzdb/time_zone_private.h"
+#include "include/tzdb/tzdb_list_private.h"
+
+// TODO TZDB remove debug printing
+#ifdef PRINT
+# include <print>
+#endif
_LIBCPP_BEGIN_NAMESPACE_STD
+#ifdef PRINT
+template <>
+struct formatter<chrono::sys_info, char> {
+ template <class ParseContext>
+ constexpr typename ParseContext::iterator parse(ParseContext& ctx) {
+ return ctx.begin();
+ }
+
+ template <class FormatContext>
+ typename FormatContext::iterator format(const chrono::sys_info& info, FormatContext& ctx) const {
+ return std::format_to(
+ ctx.out(), "[{}, {}) {:%Q%q} {:%Q%q} {}", info.begin, info.end, info.offset, info.save, info.abbrev);
+ }
+};
+#endif
+
namespace chrono {
+//===----------------------------------------------------------------------===//
+// Details
+//===----------------------------------------------------------------------===//
+
+struct __sys_info {
+ sys_info __info;
+ bool __can_merge; // Can the returned sys_info object be merged with
+};
+
+// Return type for helper function to get a sys_info.
+// - The expected result returns the "best" sys_info object. This object can be
+// before the requested time. Sometimes sys_info objects from different
+// continuations share their offset, save, and abbrev and these objects are
+// merged to one sys_info object. The __can_merge flag determines whether the
+// current result can be merged with the next result.
+// - The unexpected result means no sys_info object was found and the time is
+// the time to be used for the next search iteration.
+using __sys_info_result = expected<__sys_info, sys_seconds>;
+
+template <ranges::forward_range _Range,
+ class _Type,
+ class _Proj = identity,
+ indirect_strict_weak_order<const _Type*, projected<ranges::iterator_t<_Range>, _Proj>> _Comp = ranges::less>
+[[nodiscard]] static ranges::borrowed_iterator_t<_Range>
+__binary_find(_Range&& __r, const _Type& __value, _Comp __comp = {}, _Proj __proj = {}) {
+ auto __end = ranges::end(__r);
+ auto __ret = ranges::lower_bound(ranges::begin(__r), __end, __value, __comp, __proj);
+ if (__ret == __end)
+ return __end;
+
+ // When the value does not match the predicate it's equal and a valid result
+ // was found.
+ return !std::invoke(__comp, __value, std::invoke(__proj, *__ret)) ? __ret : __end;
+}
+
+// Format based on https://data.iana.org/time-zones/tz-how-to.html
+//
+// 1 a time zone abbreviation that is a string of three or more characters that
+// are either ASCII alphanumerics, “+”, or “-”
+// 2 the string “%z”, in which case the “%z” will be replaced by a numeric time
+// zone abbreviation
+// 3 a pair of time zone abbreviations separated by a slash (‘/’), in which
+// case the first string is the abbreviation for the standard time name and
+// the second string is the abbreviation for the daylight saving time name
+// 4 a string containing “%s”, in which case the “%s” will be replaced by the
+// text in the appropriate Rule’s LETTER column, and the resulting string should
+// be a time zone abbreviation
+//
+// Accepting invalid formats that can be processed in a sensible way would better
+// serve the user than throwing an exception. So some of these rules are not
+// strictly validated.
+// 1 This is not validated. Some examples that will be accepted are, "+04:30",
+// "Q", "42".
+// 2 How this format is formatted is not specified. In the current tzdata.zi
+// this value is not used. This value is accepted in a part of the format. So
+// "a%s%zb" will be considered valid.
+// 3 This is not validated, the output might be incorrect.
+// Proper validation would make the algorithm more complex. Then the first
+// element of the pair is used the parsing of FORMAT can stop. To do proper
+// validation the tail should be validated.
+// 4 This value is accepted in a part of the format. So "a%s%zb" will be
+// considered valid.
+[[nodiscard]] static string
+__format(const __tz::__continuation& __continuation, const string& __letters, seconds __save) {
+ bool __shift = false;
+ string __result;
+ for (char __c : __continuation.__format) {
+ if (__shift) {
+ switch (__c) {
+ case 's':
+ std::ranges::copy(__letters, std::back_inserter(__result));
+ break;
+
+ case 'z': {
+ chrono::hh_mm_ss __offset{__continuation.__stdoff + __save};
+ if (__offset.is_negative()) {
+ __result += '-';
+ __offset = chrono::hh_mm_ss{-(__continuation.__stdoff + __save)};
+ } else
+ __result += '+';
+
+ if (__offset.minutes() != 0min)
+ std::format_to(std::back_inserter(__result), "{:%H%M}", __offset);
+ else
+ std::format_to(std::back_inserter(__result), "{:%H}", __offset);
+ } break;
+
+ default:
+ std::__throw_runtime_error(
+ std::format("corrupt tzdb FORMAT field: invalid sequence '%{}' found, expected %s or %z", __c).c_str());
+ }
+ __shift = false;
+
+ } else if (__c == '/') {
+ if (__save != 0s)
+ __result.clear();
+ else
+ break;
+
+ } else if (__c == '%') {
+ __shift = true;
+ } else {
+ __result.push_back(__c);
+ }
+ }
+
+ if (__shift)
+ std::__throw_runtime_error("corrupt tzdb FORMAT field: input ended with the start of the escape sequence '%'");
+
+ return __result;
+}
+
+[[nodiscard]] static sys_seconds __to_sys_seconds(year_month_day __ymd, seconds __seconds) {
+ seconds __result = static_cast<sys_days>(__ymd).time_since_epoch() + __seconds;
+ return sys_seconds{__result};
+}
+
+[[nodiscard]] static seconds __at_to_sys_seconds(const __tz::__continuation& __continuation) {
+ switch (__continuation.__at.__clock) {
+ case __tz::__clock::__local:
+ return __continuation.__at.__time - __continuation.__stdoff -
+ std::visit(
+ [](const auto& __value) {
+ using _Tp = decay_t<decltype(__value)>;
+ if constexpr (same_as<_Tp, monostate>)
+ return chrono::seconds{0};
+ else if constexpr (same_as<_Tp, __tz::__save>)
+ return chrono::duration_cast<seconds>(__value.__time);
+ else if constexpr (same_as<_Tp, std::string>)
+ // For a named rule based continuation the SAVE depends on the RULE
+ // active at the end. This should be determined separately.
+ return chrono::seconds{0};
+ else
+ static_assert(false);
+
+ std::__libcpp_unreachable();
+ },
+ __continuation.__rules);
+
+ case __tz::__clock::__universal:
+ return __continuation.__at.__time;
+
+ case __tz::__clock::__standard:
+ return __continuation.__at.__time - __continuation.__stdoff;
+ }
+}
+
+[[nodiscard]] static year_month_day __to_year_month_day(year __year, month __month, __tz::__on __on) {
+ return std::visit(
+ [&](const auto& __value) {
+ using T = decay_t<decltype(__value)>;
+ if constexpr (same_as<T, chrono::day>)
+ return year_month_day{__year, __month, __value};
+ else if constexpr (same_as<T, weekday_last>)
+ return year_month_day{static_cast<sys_days>(year_month_weekday_last{__year, __month, __value})};
+ else if constexpr (same_as<T, __tz::__constrained_weekday>)
+ return __value(__year, __month);
+ else
+ static_assert(false);
+
+ std::__libcpp_unreachable();
+ },
+ __on);
+}
+
+[[nodiscard]] static sys_seconds __until_to_sys_seconds(const __tz::__continuation& __continuation) {
+ // Does UNTIL contain the magic value for the last continuation?
+ if (__continuation.__year == chrono::year::min())
+ return sys_seconds::max();
+
+ year_month_day __ymd = chrono::__to_year_month_day(__continuation.__year, __continuation.__in, __continuation.__on);
+ return chrono::__to_sys_seconds(__ymd, chrono::__at_to_sys_seconds(__continuation));
+}
+
+// Holds the UNTIL time for a continuation with a named rule.
+//
+// Unlike continuations with an fixed SAVE named rules have a variable SAVE.
+// This means when the UNTIL uses the local wall time the actual UNTIL value can
+// only be determined when the SAVE is known. This class holds that abstraction.
+class __named_rule_until {
+public:
+ explicit __named_rule_until(const __tz::__continuation& __continuation)
+ : __until_{chrono::__until_to_sys_seconds(__continuation)},
+ __needs_adjustment_{
+ // The last continuation of a ZONE has no UNTIL which basically is
+ // until the end of _local_ time. This value is expressed by
+ // sys_seconds::max(). Subtracting the SAVE leaves large value.
+ // However SAVE can be negative, which would add a value to maximum
+ // leading to undefined behaviour. In practice this often results in
+ // an overflow to a very small value.
+ __until_ != sys_seconds::max() && __continuation.__at.__clock == __tz::__clock::__local} {}
+
+ // Gives the unadjusted until value, this is useful when the SAVE is not known
+ // at all.
+ sys_seconds __until() const noexcept { return __until_; }
+
+ bool __needs_adjustment() const noexcept { return __needs_adjustment_; }
+
+ // Returns the UNTIL adjusted for SAVE.
+ sys_seconds operator()(seconds __save) const noexcept { return __until_ - __needs_adjustment_ * __save; }
+
+private:
+ sys_seconds __until_;
+ bool __needs_adjustment_;
+};
+
+[[nodiscard]] static seconds __at_to_seconds(seconds __stdoff, const __tz::__rule...
[truncated]
|
mordante
commented
Mar 18, 2024
| @@ -41,4 +41,5 @@ | |||
| "`4001 <https://wg21.link/LWG4001>`__","``iota_view`` should provide ``empty``","Kona November 2023","","","|ranges|" | |||
| "","","","","","" | |||
| "`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0","" | |||
| "XXXX","","The sys_info range should be affected by save","Not Yet Adopted","|Complete|","19.0" | |||
There was a problem hiding this comment.
This has been discussed with Howard and I'll file an LWG issue.
mordante
force-pushed
the
users/mordante/adds_sys_info
branch
6 times, most recently
from
96c6e86 to
7f89e68
Compare
ldionne
reviewed
Mar 26, 2024
libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/get_info.sys_time.pass.cpp
Show resolved
Hide resolved
ldionne
reviewed
Apr 2, 2024
mordante
force-pushed
the
users/mordante/completes_tzdb_class
branch
from
7a742c0 to
2e21062
Compare
mordante
force-pushed
the
users/mordante/adds_sys_info
branch
5 times, most recently
from
30e7076 to
4be4a60
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
EricWF
reviewed
Apr 7, 2024
libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp
Outdated
Show resolved
Hide resolved
EricWF
reviewed
Apr 7, 2024
| } | ||
|
|
||
| static void write(std::string_view input) { | ||
| static int version = 0; |
There was a problem hiding this comment.
That should automatically happen in the destructor. Or am I overlooking something?
EricWF
approved these changes
Apr 7, 2024
ldionne
approved these changes
Apr 9, 2024
There was a problem hiding this comment.
I reviewed this in some amount of detail, but obviously the code here is really complex so without actually reimplementing it myself, I can't understand everything. However, this looks right to me and I am relying on your test coverage a lot. I think it would be very valuable to add fuzzing tests here, I am certain it would shake out a lot of issues.
This LGTM once all pending comments have been applied. Thanks a lot for the patch!
Adds the sys_info class and time_zone::get_info(). The code still has a few quirks and has not been optimized for performance yet. The returned sys_info is compared against the output of the zdump tool in the test giving confidence the implementation is correct. Implements parts of: - P0355 Extending <chrono> to Calendars and Time Zones Implements: - LWGXXXX The sys_info range should be affected by save
mordante
force-pushed
the
users/mordante/adds_sys_info
branch
from
4be4a60 to
7fce932
Compare
|
Hi @mordante , JFI, usage of |
|
Thanks! It seems I overlooked the CI failure. I think the error can be improved, but I disagree that this is not useful. If I'd gotten a "typical" assert message I would not have been able to figure out what the error was; it wouldn't have shown 1950-01-01 00:00:00. This value helped me to discover that the I'll think about a way to improve the error to be more useful. |
|
The new tests are failing for me on Gentoo amd64: |
|
Thanks for the comment. For now I disabled the test entirely. I'll investigate the later. |
|
Hi @mordante ,
No, my point was that it is difficult to get the exact problem, especially without accessing to the target equpment. Sorry for unclear explanation. Anyway, I tried to find which part of the test is failed and I found that it gets failed on There is a zdump for aarch64 / Ubuntu Linux 22.04 (passed test) aarch64 / Ubuntu Linux 18.04 armv7 / Ubuntu Linux 18.04 (FAILED test) Hope it will help in your investigation of the problem. |
|
Hm, even more interesting. On my Gentoo system, amd64, glibc 2.39, tzdata 2024a: |
Ah thanks. I'm working on a patch to show the source location of the caller, which would make it a lot easier to spot the issue. > Hope it will help in your investigation of the problem. Thanks I think it will. I'll check whether these rules have changes, in which case it might depend on the version of the time zone database used. But not only the version seems to matter; there seem to be configuration options too. 2024a on Debian Stable works, but Debian sid is not compatible. (That is a different story which I'm working on.) For the short term I keep this test disabled and will investigate this further; I first want to improve the output of the error messages. |
|
Hi @mordante , I encounter some test failure with sys_info.zdump.pass.cpp in different OS. Here are some os, tzdata version and zdump version first:
First: Second:
I don't know much about chrono, hope above information is useful. If you need more information, please let me know. |
|
Thanks for the info @yingcong-wu. This looks odd, both zdump and libc++ should use the same tzdata.zi and thus produce identical results. I'm really curious about the output of |
|
Sure, it is The Ubuntu 24.04 is the same. |
|
Thanks. This looks really odd. UTC should not have any changes. This looks like UTC has daylight-saving time. This is especially odd since the libc++ CI also uses Ubuntu 22.04. Just to be clear, this is the output of zdump. Is that correct? |
Yes, those are the output of But it is weird that I use another Ubuntu 22.04 that has the same tzdata.zi with Could that be related to kernel? This one with non-empty output is running on a container with |
|
I don't see how this can be related to the kernel. There might be a different software package that affects the result. |
|
I'm looking at moving Linaro's bots to Ubuntu Jammy and I think I can confirm this is not a kernel issue. On armv8l Ubuntu Jammy this is a UPASS. On armv8l Ubuntu focal it fails as expected: On AArch64 it passes on Focal and Jammy. What else could I compare to figure out this difference? I think some |
|
Interesting it seems zdump is part of glibc. I thought it was provided by the time zone related packages. |
|
The 2.35 output has more entries for many of the zones. Focal: Jammy: According to @zatrazz this may be because in glibc 2.34, the armhf version of zdump started to be built with 64 bit time_t support. Quoting them: They suggest that using a value that will fit into 32 bit time_t is one possibility, but I think the point of the test is to use a very wide range: Presumably no one is ever going to edit the 1869 entries, so I will try writing a ore-test check to make sure zdump works by looking for those entries. |
|
How do you deal with this issue I'm seeing on RHEL8?: while while on all other Linux distros it's in |
|
The test is marked with: Which is checked here: So |
|
Ok, so with sudo I've copied |
|
I would also check how std::system handles environment variables and paths. Perhaps the way /usr/sbin is added during a normal terminal session doesn't carry through. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds the sys_info class and time_zone::get_info(). The code still has a few quirks and has not been optimized for performance yet.
The returned sys_info is compared against the output of the zdump tool in the test giving confidence the implementation is correct.
Implements parts of:
Implements: