[libc++][chrono] implements UTC clock. #90393
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) ChangesWhile implementing this feature and its associated LWG issues it turns out
Implements parts of:
Patch is 115.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90393.diff 26 Files Affected:
diff --git a/libcxx/benchmarks/CMakeLists.txt b/libcxx/benchmarks/CMakeLists.txt
index 527a2acf2d3b36..ea7ef6902f7266 100644
--- a/libcxx/benchmarks/CMakeLists.txt
+++ b/libcxx/benchmarks/CMakeLists.txt
@@ -229,6 +229,7 @@ set(BENCHMARK_TESTS
system_error.bench.cpp
to_chars.bench.cpp
unordered_set_operations.bench.cpp
+ utc_clock.bench.cpp
util_smartptr.bench.cpp
variant_visit_1.bench.cpp
variant_visit_2.bench.cpp
diff --git a/libcxx/benchmarks/utc_clock.bench.cpp b/libcxx/benchmarks/utc_clock.bench.cpp
new file mode 100644
index 00000000000000..255b1dde52aa67
--- /dev/null
+++ b/libcxx/benchmarks/utc_clock.bench.cpp
@@ -0,0 +1,54 @@
+//===----------------------------------------------------------------------===//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include <chrono>
+
+#include "benchmark/benchmark.h"
+
+// Benchmarks the performance of the UTC <-> system time conversions. These
+// operations determine the sum of leap second insertions at a specific time.
+
+static void BM_from_sys(benchmark::State& state) {
+ std::chrono::sys_days date{std::chrono::July / 1 / state.range(0)};
+ for (auto _ : state)
+ benchmark::DoNotOptimize(std::chrono::utc_clock::from_sys(date));
+}
+
+BENCHMARK(BM_from_sys)
+ ->Arg(1970) // before the first leap seconds
+ ->Arg(1979) // in the first half of inserted leap seconds
+ ->Arg(1993) // in the second half of inserted leap seconds
+ ->Arg(2100); // after the last leap second
+
+BENCHMARK(BM_from_sys)->Arg(1970)->Arg(1979)->Arg(1993)->Arg(2100)->Threads(4);
+BENCHMARK(BM_from_sys)->Arg(1970)->Arg(1979)->Arg(1993)->Arg(2100)->Threads(16);
+
+static void BM_to_sys(benchmark::State& state) {
+ // 59 sec offset means we pass th UTC offset for the leap second; assuming
+ // there won't be more than 59 leap seconds ever.
+ std::chrono::utc_seconds date{
+ std::chrono::sys_days{std::chrono::July / 1 / state.range(0)}.time_since_epoch() + std::chrono::seconds{59}};
+ for (auto _ : state)
+ benchmark::DoNotOptimize(std::chrono::utc_clock::to_sys(date));
+}
+
+BENCHMARK(BM_to_sys)
+ ->Arg(1970) // before the first leap seconds
+ ->Arg(1979) // in the first half of inserted leap seconds
+ ->Arg(1993) // in the second half of inserted leap seconds
+ ->Arg(2100); // after the last leap second
+
+BENCHMARK(BM_to_sys)->Arg(1970)->Arg(1979)->Arg(1993)->Arg(2100)->Threads(4);
+BENCHMARK(BM_to_sys)->Arg(1970)->Arg(1979)->Arg(1993)->Arg(2100)->Threads(16);
+
+int main(int argc, char** argv) {
+ benchmark::Initialize(&argc, argv);
+ if (benchmark::ReportUnrecognizedArguments(argc, argv))
+ return 1;
+
+ benchmark::RunSpecifiedBenchmarks();
+}
diff --git a/libcxx/docs/Status/Cxx20.rst b/libcxx/docs/Status/Cxx20.rst
index 23289dc6e5961b..baa1dc6d613826 100644
--- a/libcxx/docs/Status/Cxx20.rst
+++ b/libcxx/docs/Status/Cxx20.rst
@@ -55,9 +55,9 @@ Paper Status
* ``Input parsers`` not done
* ``Stream output`` Obsolete due to `P1361R2 <https://wg21.link/P1361R2>`_ "Integration of chrono with text formatting"
* ``Time zone and leap seconds`` In Progress
+ * ``UTC clock`` Clang 19
* ``TAI clock`` not done
* ``GPS clock`` not done
- * ``UTC clock`` not done
.. [#note-P0718] P0718: Implemented deprecation of ``shared_ptr`` atomic access APIs only.
diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index fed1c7e736248f..556da4fc72f83a 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -238,7 +238,7 @@
"`3313 <https://wg21.link/LWG3313>`__","``join_view::iterator::operator--``\ is incorrectly constrained","Prague","|Complete|","14.0","|ranges|"
"`3314 <https://wg21.link/LWG3314>`__","Is stream insertion behavior locale dependent when ``Period::type``\ is ``micro``\ ?","Prague","|Complete|","16.0","|chrono|"
"`3315 <https://wg21.link/LWG3315>`__","Correct Allocator Default Behavior","Prague","",""
-"`3316 <https://wg21.link/LWG3316>`__","Correctly define epoch for ``utc_clock``\ / ``utc_timepoint``\ ","Prague","","","|chrono|"
+"`3316 <https://wg21.link/LWG3316>`__","Correctly define epoch for ``utc_clock``\ / ``utc_timepoint``\ ","Prague","|Nothing To Do|","","|chrono|"
"`3317 <https://wg21.link/LWG3317>`__","Incorrect ``operator<<``\ for floating-point durations","Prague","","","|chrono|"
"`3318 <https://wg21.link/LWG3318>`__","Clarify whether clocks can represent time before their epoch","Prague","","","|chrono|"
"`3319 <https://wg21.link/LWG3319>`__","Properly reference specification of IANA time zone database","Prague","|Nothing To Do|","","|chrono|"
diff --git a/libcxx/docs/Status/FormatPaper.csv b/libcxx/docs/Status/FormatPaper.csv
index f29f1f7ca74875..bf733e264cde22 100644
--- a/libcxx/docs/Status/FormatPaper.csv
+++ b/libcxx/docs/Status/FormatPaper.csv
@@ -2,7 +2,7 @@ Section,Description,Dependencies,Assignee,Status,First released version
`P1361 <https://wg21.link/P1361>`__ `P2372 <https://wg21.link/P2372>`__,"Formatting chrono"
`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::duration<Rep, Period>``",,Mark de Wever,|Complete|,16.0
`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::sys_time<Duration>``",,Mark de Wever,|Complete|,17.0
-`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::utc_time<Duration>``",A ``<chrono>`` implementation,Mark de Wever,,,
+`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::utc_time<Duration>``",,Mark de Wever,|Complete|,19.0
`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::tai_time<Duration>``",A ``<chrono>`` implementation,Mark de Wever,,,
`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::gps_time<Duration>``",A ``<chrono>`` implementation,Mark de Wever,,,
`[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::file_time<Duration>``",,Mark de Wever,|Complete|,17.0
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 1296c536bc882c..4aff1487030500 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -285,6 +285,7 @@ set(files
__chrono/time_zone_link.h
__chrono/tzdb.h
__chrono/tzdb_list.h
+ __chrono/utc_clock.h
__chrono/weekday.h
__chrono/year.h
__chrono/year_month.h
diff --git a/libcxx/include/__chrono/convert_to_tm.h b/libcxx/include/__chrono/convert_to_tm.h
index 881a4970822d8e..8e9310da2ce94a 100644
--- a/libcxx/include/__chrono/convert_to_tm.h
+++ b/libcxx/include/__chrono/convert_to_tm.h
@@ -24,6 +24,7 @@
#include <__chrono/sys_info.h>
#include <__chrono/system_clock.h>
#include <__chrono/time_point.h>
+#include <__chrono/utc_clock.h>
#include <__chrono/weekday.h>
#include <__chrono/year.h>
#include <__chrono/year_month.h>
@@ -96,6 +97,21 @@ _LIBCPP_HIDE_FROM_ABI _Tm __convert_to_tm(const chrono::sys_time<_Duration> __tp
return __result;
}
+# if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
+ !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+template <class _Tm, class _Duration>
+_LIBCPP_HIDE_FROM_ABI _Tm __convert_to_tm(chrono::utc_time<_Duration> __tp) {
+ _Tm __result = std::__convert_to_tm<_Tm>(chrono::utc_clock::to_sys(__tp));
+
+ if (chrono::get_leap_second_info(__tp).is_leap_second)
+ ++__result.tm_sec;
+
+ return __result;
+}
+
+# endif // !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
+ // !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
// Convert a chrono (calendar) time point, or dururation to the given _Tm type,
// which must have the same properties as std::tm.
template <class _Tm, class _ChronoT>
@@ -108,6 +124,8 @@ _LIBCPP_HIDE_FROM_ABI _Tm __convert_to_tm(const _ChronoT& __value) {
if constexpr (__is_time_point<_ChronoT>) {
if constexpr (same_as<typename _ChronoT::clock, chrono::system_clock>)
return std::__convert_to_tm<_Tm>(__value);
+ else if constexpr (same_as<typename _ChronoT::clock, chrono::utc_clock>)
+ return std::__convert_to_tm<_Tm>(__value);
else if constexpr (same_as<typename _ChronoT::clock, chrono::file_clock>)
return std::__convert_to_tm<_Tm>(_ChronoT::clock::to_sys(__value));
else if constexpr (same_as<typename _ChronoT::clock, chrono::local_t>)
diff --git a/libcxx/include/__chrono/formatter.h b/libcxx/include/__chrono/formatter.h
index e9b81c3de8a700..6a6f60341c1394 100644
--- a/libcxx/include/__chrono/formatter.h
+++ b/libcxx/include/__chrono/formatter.h
@@ -28,6 +28,7 @@
#include <__chrono/sys_info.h>
#include <__chrono/system_clock.h>
#include <__chrono/time_point.h>
+#include <__chrono/utc_clock.h>
#include <__chrono/weekday.h>
#include <__chrono/year.h>
#include <__chrono/year_month.h>
@@ -673,6 +674,23 @@ struct _LIBCPP_TEMPLATE_VIS formatter<chrono::sys_time<_Duration>, _CharT> : pub
}
};
+# if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
+ !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
+template <class _Duration, __fmt_char_type _CharT>
+struct _LIBCPP_TEMPLATE_VIS formatter<chrono::utc_time<_Duration>, _CharT> : public __formatter_chrono<_CharT> {
+public:
+ using _Base = __formatter_chrono<_CharT>;
+
+ template <class _ParseContext>
+ _LIBCPP_HIDE_FROM_ABI constexpr typename _ParseContext::iterator parse(_ParseContext& __ctx) {
+ return _Base::__parse(__ctx, __format_spec::__fields_chrono, __format_spec::__flags::__clock);
+ }
+};
+
+# endif // !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
+ // !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
template <class _Duration, __fmt_char_type _CharT>
struct _LIBCPP_TEMPLATE_VIS formatter<chrono::file_time<_Duration>, _CharT> : public __formatter_chrono<_CharT> {
public:
diff --git a/libcxx/include/__chrono/ostream.h b/libcxx/include/__chrono/ostream.h
index ecf07a320c8b94..4862a8ae326a3e 100644
--- a/libcxx/include/__chrono/ostream.h
+++ b/libcxx/include/__chrono/ostream.h
@@ -22,6 +22,7 @@
#include <__chrono/statically_widen.h>
#include <__chrono/sys_info.h>
#include <__chrono/system_clock.h>
+#include <__chrono/utc_clock.h>
#include <__chrono/weekday.h>
#include <__chrono/year.h>
#include <__chrono/year_month.h>
@@ -56,6 +57,17 @@ operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_days& __dp) {
return __os << year_month_day{__dp};
}
+# if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
+ !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
+template <class _CharT, class _Traits, class _Duration>
+_LIBCPP_HIDE_FROM_ABI basic_ostream<_CharT, _Traits>&
+operator<<(basic_ostream<_CharT, _Traits>& __os, const utc_time<_Duration>& __tp) {
+ return __os << std::format(__os.getloc(), _LIBCPP_STATICALLY_WIDEN(_CharT, "{:L%F %T}"), __tp);
+}
+# endif // !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
+ // !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+
template <class _CharT, class _Traits, class _Duration>
_LIBCPP_HIDE_FROM_ABI basic_ostream<_CharT, _Traits>&
operator<<(basic_ostream<_CharT, _Traits>& __os, const file_time<_Duration> __tp) {
diff --git a/libcxx/include/__chrono/utc_clock.h b/libcxx/include/__chrono/utc_clock.h
new file mode 100644
index 00000000000000..a3db972f26a4e2
--- /dev/null
+++ b/libcxx/include/__chrono/utc_clock.h
@@ -0,0 +1,155 @@
+// -*- 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_UTC_CLOCK_H
+#define _LIBCPP___CHRONO_UTC_CLOCK_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 <__chrono/tzdb.h>
+# include <__chrono/tzdb_list.h>
+# include <__config>
+# include <__type_traits/common_type.h>
+
+# 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 {
+
+class utc_clock;
+
+template <class _Duration>
+using utc_time = time_point<utc_clock, _Duration>;
+using utc_seconds = utc_time<seconds>;
+
+class utc_clock {
+public:
+ using rep = system_clock::rep;
+ using period = system_clock::period;
+ using duration = chrono::duration<rep, period>;
+ using time_point = chrono::time_point<utc_clock>;
+ static constexpr bool is_steady = false; // The system_clock is not steady.
+
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI static time_point now() { return from_sys(system_clock::now()); }
+
+ template <class _Duration>
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI static sys_time<common_type_t<_Duration, seconds>>
+ to_sys(const utc_time<_Duration>& __time);
+
+ template <class _Duration>
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI static utc_time<common_type_t<_Duration, seconds>>
+ from_sys(const sys_time<_Duration>& __time) {
+ using _Rp = utc_time<common_type_t<_Duration, seconds>>;
+ // TODO TZDB investigate optimizations.
+ //
+ // The leap second database stores all transitions, this mean to calculate
+ // the current number of leap seconds the code needs to iterate over all
+ // leap seconds to accumulate the sum. Then the sum can be used to determine
+ // the sys_time. Accessing the database involves acquiring a mutex.
+ //
+ // The historic entries in the database are immutable. Hard-coding these
+ // values in a table would allow:
+ // - To store the sum, allowing a binary search on the data.
+ // - Avoid acquiring a mutex.
+ // The disadvantage are:
+ // - A slightly larger code size.
+ //
+ // There are two optimization directions
+ // - hard-code the database and do a linear search for future entries. This
+ // search can start at the back, and should probably contain very few
+ // entries. (Adding leap seconds is quite rare and new release of libc++
+ // can add the new entries; they are announced half a year before they are
+ // added.)
+ // - During parsing the leap seconds store an additional database in the
+ // dylib with the list of the sum of the leap seconds. In that case there
+ // can be a private function __get_utc_to_sys_table that returns the
+ // table.
+ //
+ // Note for to_sys there are no optimizations to be done; it uses
+ // get_leap_second_info. The function get_leap_second_info could benefit
+ // from optimizations as described above; again both options apply.
+
+ // Both UTC and the system clock use the same epoch. The Standard
+ // specifies from 1970-01-01 even when UTC starts at
+ // 1972-01-01 00:00:10 TAI. So when the sys_time is before epoch we can be
+ // sure there both clocks return the same value.
+
+ const tzdb& __tzdb = chrono::get_tzdb();
+ _Rp __result{__time.time_since_epoch()};
+ for (const auto& __leap_second : __tzdb.leap_seconds) {
+ if (__time < __leap_second)
+ return __result;
+
+ __result += __leap_second.value();
+ }
+ return __result;
+ }
+};
+
+struct leap_second_info {
+ bool is_leap_second;
+ seconds elapsed;
+};
+
+template <class _Duration>
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI leap_second_info get_leap_second_info(const utc_time<_Duration>& __time) {
+ const tzdb& __tzdb = chrono::get_tzdb();
+ if (__tzdb.leap_seconds.empty())
+ return {false, chrono::seconds{0}};
+
+ sys_seconds __sys{chrono::floor<seconds>(__time).time_since_epoch()};
+ seconds __elapsed{0};
+ for (const auto& __leap_second : __tzdb.leap_seconds) {
+ if (__sys == __leap_second.date() + __elapsed)
+ return {__leap_second.value() > 0s, // only positive leap seconds are considered leap seconds
+ __elapsed + __leap_second.value()};
+
+ if (__sys < __leap_second.date() + __elapsed)
+ return {false, __elapsed};
+
+ __elapsed += __leap_second.value();
+ }
+
+ return {false, __elapsed};
+}
+
+template <class _Duration>
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI sys_time<common_type_t<_Duration, seconds>>
+utc_clock::to_sys(const utc_time<_Duration>& __time) {
+ using _Dp = common_type_t<_Duration, seconds>;
+ leap_second_info __info = get_leap_second_info(__time);
+
+ sys_time<common_type_t<_Duration, seconds>> __result{__time.time_since_epoch() - __info.elapsed};
+ if (__info.is_leap_second)
+ return chrono::floor<seconds>(__result) + chrono::seconds{1} - _Dp{1};
+
+ return __result;
+}
+
+} // 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_UTC_CLOCK_H
diff --git a/libcxx/include/chrono b/libcxx/include/chrono
index 4d8398af1a108f..7ce527f120e9e4 100644
--- a/libcxx/include/chrono
+++ b/libcxx/include/chrono
@@ -300,6 +300,41 @@ template<class charT, class traits> // C++20
basic_ostream<charT, traits>&
operator<<(basic_ostream<charT, traits>& os, const sys_days& dp);
+// [time.clock.utc], class utc_clock
+class utc_clock { // C++20
+public:
+ using rep = a signed arithmetic type;
+ using period = ratio<unspecified, unspecified>;
+ using duration = chrono::duration<rep, period>;
+ using time_point = chrono::time_point<utc_clock>;
+ static constexpr bool is_steady = unspecified;
+
+ static time_point now();
+
+ template<class Duration>
+ static sys_time<common_type_t<Duration, seconds>>
+ to_sys(const utc_time<Duration>& t);
+ template<class Duration>
+ static utc_time<common_type_t<Duration, seconds>>
+ from_sys(const sys_time<Duration>& t);
+};
+
+template<class Duration>
+using utc_time = time_point<utc_clock, Duration>; // C++20
+using utc_seconds = utc_time<seconds>; // C++20
+
+template<class charT, class traits, class Duration> // C++20
+ basic_ostream<charT, traits>&
+ operator<<(basic_ostream<charT, traits>& os, const utc_time<Duration>& t);
+
+struct leap_second_info { // C++20
+ bool is_leap_second;
+ seconds elapsed;
+};
+
+template<class Duration> // C++20
+ leap_second_info get_leap_second_info(const utc_time<Duration>& ut);
+
class file_clock // C++20
{
public:
@@ -827,6 +862,8 @@ strong_ordering operator<=>(const time_zone_link& x, const time_zone_link& y);
namespace std {
template<class Duration, class charT>
struct formatter<chrono::sys_time<Duration>, charT>; // C++20
+ template<class Duration, class charT>
+ struct formatter<chrono::utc_time<Duration>, charT>; // C++20
template<class Duration, class charT>
struct formatter<chrono::filetime<Duration>, charT>; // C++20
template<class Duration, class charT>
@@ -942,6 +979,7 @@ constexpr chrono::year operator ""y(unsigned lo
# include <__chrono/time_zone_link.h>
# include <__chrono/tzdb.h>
# include <__chrono/tzdb_list.h>
+# include <__chrono/utc_clock.h>
# endif
#endif
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index...
[truncated]
|
mordante
force-pushed
the
users/mordante/implements_utc_clock
branch
from
188e6f4 to
77edef4
Compare
|
|
||
| sys_time<common_type_t<_Duration, seconds>> __result{__time.time_since_epoch() - __info.elapsed}; | ||
| if (__info.is_leap_second) | ||
| return chrono::floor<seconds>(__result) + chrono::seconds{1} - _Dp{1}; |
There was a problem hiding this comment.
I don't think this is right for floating point duration reps, where the ulp can be more or less than _Dp{1}. I think nextafter is needed for reps where treat_as_floating_point_v is true. (Assuming you have such an overload for extended-precision floats. I'm also uncertain if [time.duration.general]/2 allows a user-defined type to "emulate" a floating point type by specializing treat_as_floating_point.)
There was a problem hiding this comment.
Good point. I'll need to look into this.
There was a problem hiding this comment.
I've added some tests with floating point, but I don't see an issue with the existing code. Do you have an example of problematic values?
There was a problem hiding this comment.
You can't use float because the ulp is not less than 1 in the relevant range, so it's difficult to observe the effect. Also, the test does the same thing as the product code, which is subtract 1, but I don't think that's correct. For example, with the first leap second (78796800s) and double precision, I think the answer should be S{D{nextafter(78796800.0, 0.0)}}, i.e. 0x1.2c95fffffffffp+26, for any UTC time during the leap second insertion. To me, that's the "last representable value" prior to the insertion.
libcxx/test/libcxx/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp
Show resolved
Hide resolved
| } | ||
|
|
||
| template <class CharT> | ||
| static void test_utc_transitions() { |
There was a problem hiding this comment.
If you can customize the leap second db like in some of the other tests, it might be useful to verify that a negative leap second has the correct behavior, i.e., "23:59:58" is followed by "00:00:00".
There was a problem hiding this comment.
I thought I had such a test, but seems only with my own faked data. I'll look at adding such a test and I want to look at using the fake numbers Paul Eggert generated. Thanks for that link!
mordante
commented
May 1, 2024
|
|
||
| sys_time<common_type_t<_Duration, seconds>> __result{__time.time_since_epoch() - __info.elapsed}; | ||
| if (__info.is_leap_second) | ||
| return chrono::floor<seconds>(__result) + chrono::seconds{1} - _Dp{1}; |
There was a problem hiding this comment.
Good point. I'll need to look into this.
libcxx/test/libcxx/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp
Show resolved
Hide resolved
mordante
force-pushed
the
users/mordante/fixes_leap_second_database_format
branch
from
d094cde to
9e551c1
Compare
Base automatically changed from
users/mordante/fixes_leap_second_database_format
to
main
July 4, 2024 18:45
mordante
force-pushed
the
users/mordante/implements_utc_clock
branch
from
77edef4 to
3fd4c66
Compare
You can test this locally with the following command:git-clang-format --diff b7abc510c515c4df521c84c6f664a138f8cf01e0 f7d3064842caeb4c6716d92e05073c5c2740c5ed --extensions cpp,,h,inc -- libcxx/include/__chrono/utc_clock.h libcxx/test/benchmarks/utc_clock.bench.cpp libcxx/test/libcxx/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp libcxx/test/libcxx/time/time.clock/time.clock.utc/time.clock.utc.members/from_sys.pass.cpp libcxx/test/libcxx/time/time.clock/time.clock.utc/time.clock.utc.members/to_sys.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/leap_second_info.members.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/from_sys.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/now.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/to_sys.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/types.compile.pass.cpp libcxx/test/std/time/time.clock/time.clock.utc/utc_time.ostream.pass.cpp libcxx/test/std/time/time.syn/formatter.utc_time.pass.cpp libcxx/include/__chrono/convert_to_tm.h libcxx/include/__chrono/formatter.h libcxx/include/__chrono/ostream.h libcxx/include/chrono libcxx/modules/std/chrono.inc libcxx/test/libcxx/diagnostics/chrono.nodiscard.verify.cpp libcxx/test/std/utilities/format/format.formattable/concept.formattable.compile.pass.cppView the diff from clang-format here.diff --git a/libcxx/test/std/utilities/format/format.formattable/concept.formattable.compile.pass.cpp b/libcxx/test/std/utilities/format/format.formattable/concept.formattable.compile.pass.cpp
index 88a485a256..49d3e756e3 100644
--- a/libcxx/test/std/utilities/format/format.formattable/concept.formattable.compile.pass.cpp
+++ b/libcxx/test/std/utilities/format/format.formattable/concept.formattable.compile.pass.cpp
@@ -156,7 +156,7 @@ void test_P1361() {
//assert_is_formattable<std::chrono::tai_time<std::chrono::microseconds>, CharT>();
//assert_is_formattable<std::chrono::gps_time<std::chrono::microseconds>, CharT>();
-# endif // !defined(TEST_HAS_NO_EXPERIMENTAL_TZDB) && !defined(TEST_HAS_NO_TIME_ZONE_DATABASE) &&
+# endif // !defined(TEST_HAS_NO_EXPERIMENTAL_TZDB) && !defined(TEST_HAS_NO_TIME_ZONE_DATABASE) && \
// !defined(TEST_HAS_NO_FILESYSTEM)
assert_is_formattable<std::chrono::file_time<std::chrono::microseconds>, CharT>();
|
ldionne
requested changes
Jul 16, 2024
libcxx/test/std/time/time.clock/time.clock.utc/utc_time.ostream.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/time/time.clock/time.clock.utc/types.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/libcxx/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/time/time.clock/time.clock.utc/leap_second_info.members.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/now.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/to_sys.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/to_sys.pass.cpp
Show resolved
Hide resolved
ldionne
reviewed
Dec 10, 2024
libcxx/test/std/time/time.clock/time.clock.utc/leap_second_info.members.pass.cpp
Show resolved
Hide resolved
|
|
||
| sys_time<common_type_t<_Duration, seconds>> __result{__time.time_since_epoch() - __info.elapsed}; | ||
| if (__info.is_leap_second) | ||
| return chrono::floor<seconds>(__result) + chrono::seconds{1} - _Dp{1}; |
There was a problem hiding this comment.
Floating-point concerns aside, I don't understand how this works. My (naive) understanding is that _Dp can be smaller than seconds, for example if common_type_t<_Duration, seconds> is chrono::milliseconds. In that case, you'd be flooring the result, then adding 1 second and subtracting one millisecond. So basically you'd be adding 0.999 seconds to the result. I don't understand why that is correct. If that's a bug, let's add a test!
There was a problem hiding this comment.
The relevant language is [time.clock.utc.members]/2: "[If u represents a time during a leap second insertion,] the last representable value of sys_time prior to the insertion of the leap second is returned."
Briefly, I think the part you might be missing is that leap_second_info::elapsed is being subtracted before adding seconds{1} - _Dp{1}, and elapsed is incremented as soon as the insertion begins. So in this branch, when is_leap_second is true, floor<seconds>(__result) is one second before the leap second insertion began.
To illustrate, suppose that we're examining a day with a positive leap second. To make things easier, assume that the UTC epoch is midnight of that day, this is the first leap second ever, and the _Dpratio isratio<1,4>`. Then:
| UTC time | __time |
__info |
__result |
floor<seconds>(__result) + seconds{1} - _Dp{1} |
Returned system time |
|---|---|---|---|---|---|
| 23:59:59.75 | 345,599 | {false, 0s} | 345,599 | N/A | 23:59:59.75 |
| 23:59:60.00 | 345,600 | {true,1s} | 345,596 | 345,599 (=345,596+4-1) | 23:59:59.75 |
| 23:59:60.25 | 345,601 | {true,1s} | 345,597 | 345,599 | 23:59:59.75 |
| 23:59:60.50 | 345,602 | {true,1s} | 345,598 | 345,599 | 23:59:59.75 |
| 23:59:60.75 | 345,603 | {true,1s} | 345,599 | 345,599 | 23:59:59.75 |
| 00:00:00.00 | 345,604 | {false,1s} | 345,600 | N/A | 00:00:00.00 |
There was a problem hiding this comment.
@MattStephanson I read your comment, but I'm not sure what the issue is. "the last representable value of sys_time prior to the insertion of the leap second is returned." Looking at the floor<seconds>(__result) + seconds{1} - _Dp{1} and "Returned system time" columns these are the values returned. Could you show the values you would have expected?
There was a problem hiding this comment.
Thanks a lot for the explanation, this makes a lot more sense now!
libcxx/test/std/time/time.clock/time.clock.utc/time.clock.utc.members/from_sys.pass.cpp
Outdated
Show resolved
Hide resolved
mordante
force-pushed
the
users/mordante/implements_utc_clock
branch
2 times, most recently
from
b7044a0 to
1dc7845
Compare
mordante
force-pushed
the
users/mordante/implements_utc_clock
branch
7 times, most recently
from
c31fcda to
3f47c8f
Compare
ldionne
approved these changes
Jan 21, 2025
|
|
||
| sys_time<common_type_t<_Duration, seconds>> __result{__time.time_since_epoch() - __info.elapsed}; | ||
| if (__info.is_leap_second) | ||
| return chrono::floor<seconds>(__result) + chrono::seconds{1} - _Dp{1}; |
There was a problem hiding this comment.
Thanks a lot for the explanation, this makes a lot more sense now!
There was a problem hiding this comment.
How long does this benchmark take? I'm trying to keep benchmarks pretty cheap to run so that we can move towards running them automatically on a regular basis.
libcxx/test/libcxx/time/time.clock/time.clock.utc/get_leap_second_info.pass.cpp
Show resolved
Hide resolved
mordante
force-pushed
the
users/mordante/implements_utc_clock
branch
3 times, most recently
from
01fe3f9 to
ecf9334
Compare
While implementing this feature and its associated LWG issues it turns out - LWG3316 Correctly define epoch for utc_clock / utc_timepoint only added non-normative wording to the standard. Implements parts of: - P0355 Extending <chrono> to Calendars and Time Zones - P1361 Integration of chrono with text formatting - LWG3359 <chrono> leap second support should allow for negative leap seconds
mordante
force-pushed
the
users/mordante/implements_utc_clock
branch
from
ecf9334 to
f7d3064
Compare
|
The formatting error is ignored; there seems to be an issue in clang-format where it formats things in a way it makes GCC unhappy. (The issue is in the test code, not in the library code.) |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/6086 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/193/builds/5195 Here is the relevant piece of the build log for the reference |
|
Heads-up; this is probably causing an infinite loop in the compiler, where clang just halts forever. Working on a repro. |
False alarm, this patch caused a failure that masked another in the root-causing.. I'll try to understand the relation between the culprits first. |
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.
While implementing this feature and its associated LWG issues it turns out
Implements parts of: