<chrono>: C++20 clocks, clock_cast, tzdb::leap_seconds #1671
Conversation
| static constexpr bool is_steady = system_clock::is_steady; | ||
|
|
||
| _NODISCARD static time_point now() { | ||
| return from_sys(system_clock::now()); |
There was a problem hiding this comment.
This is inaccurate during a leap second.
There was a problem hiding this comment.
It's allowable by [time.clock.utc.members]/1, but would it be acceptable to return a fixed offset from GetSystemTimePreciseAsFileTime, even though systems prior to Server 2019/Windows 10 never count leap seconds? I'm currently doing that with file_clock, but now I'm having second thoughts. Maybe an #if _STL_WIN32_WINNT >= _STL_WIN32_WINNT_WIN10?
stl/inc/chrono
Outdated
| if constexpr (is_integral_v<typename _Duration::rep>) { | ||
| constexpr auto _Delta{seconds{1} - _CT{1}}; | ||
| _Ticks = _Leap_sec_minus_one + _Delta; | ||
| } else { | ||
| const auto _Leap_sec_begin = _CHRONO ceil<_CT>(_Leap_sec_minus_one + seconds{1}); | ||
| _Ticks = _CT{_STD nextafter(_Leap_sec_begin.count(), typename _CT::rep{0})}; | ||
| } |
There was a problem hiding this comment.
duration::rep could be a custom arithmetic-like type. <chrono> algorithms are expected to determine whether rep should behave like an integral type or a floating-point type by treat_as_floating_point_v<rep>.
This also means that we can't call nextafter if rep is a custom real-like type.
There was a problem hiding this comment.
Unfortunately, I'm running out of bandwidth to try to figure out what would be the least wrong thing here. My current best idea is to restrict the nextafter branch to is_floating_point_v types and for other non-integral types return _CHRONO floor<_CT>(_Leap_sec_minus_one). I don't have any ideas about getting a closer result without risking rounding up to _Leap_sec_minus_one + 1s or greater.
There was a problem hiding this comment.
Thanks @statementreply for noticing this subtle conformance issue. I think it's fine not to handle custom arithmetic types, even for the final merge of feature/chrono into main - @mnatsuhara is noting this with a card in the Chrono Project, and we'll file a tracking issue if necessary (if it's not resolved before the final merge). Basically I think that no users will encounter this in practice, so it's okay to defer - especially because it'll just be a compiler error.
There was a problem hiding this comment.
I've reviewed everything except for the additions to P0355R7_calendars_and_time_zones_clocks/test.cpp - awesome job here! Thanks for all of your hard work :)
I believe I've updated the chrono project with everything that is included in this PR (though I'm not actually clicking the checkboxes until this gets checked in).
| template <class _Ty, class _Clock> | ||
| concept _Is_time_point = requires { | ||
| typename _Ty::duration; | ||
| requires is_same_v<time_point<_Clock, typename _Ty::duration>, _Ty>; | ||
| }; | ||
|
|
||
| template <class _Clock, class _Duration> | ||
| concept _Convertible_to_sys_time = | ||
| is_same_v<_Clock, system_clock> || requires(const time_point<_Clock, _Duration>& _Time) { | ||
| { _Clock::to_sys(_Time) } | ||
| ->_Is_time_point<system_clock>; | ||
| }; | ||
|
|
||
| template <class _Clock, class _Duration> | ||
| concept _Convertible_from_sys_time = is_same_v<_Clock, system_clock> || requires(const sys_time<_Duration>& _Time) { | ||
| { _Clock::from_sys(_Time) } | ||
| ->_Is_time_point<_Clock>; | ||
| }; |
There was a problem hiding this comment.
I believe we should be guarding concepts-related code with #ifdef __cpp_lib_concepts since EDG does not yet fully support concepts. We'll also likely need to toggle on/off clang-format since it does not yet deal with concepts nicely.
There was a problem hiding this comment.
These pass the x86 /BE tests, but if that's not good enough either I'll have to rewrite these with enable_if of guard all of the clock_cast machinery. I don't really understand the implications of blocking out such a large block for EDG, so I'm not sure which approach would be better.
There was a problem hiding this comment.
Maybe I'm mistaken then! Passing /BE tests is definitely a good sign -- @StephanTLavavej may be able to shed some more light on this when he reviews this PR later!
|
Apologies for the flurry of pushes here -- I tried to get the toolset update, spaceship changes, etc. by merging with I have reset to the last commit that was actually in this PR and the files changed look right to me now. I'll merge with |
tests/std/tests/P0355R7_calendars_and_time_zones_clocks/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0355R7_calendars_and_time_zones_clocks/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0355R7_calendars_and_time_zones_clocks/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0355R7_calendars_and_time_zones_clocks/test.cpp
Outdated
Show resolved
Hide resolved
Added link to issue: Works toward #12
A few comments on design choices:
vector<leap_second>, I put in enough of [time.zone] to get that data through the Standard interface. If it's undesirable to present an incomplete interface to users, I can uglify the identifiers.tzdb_listobject. I could go back to storing astatic void*in the import lib if that's ultimately preferable, though. I tried to mitigate the impact by using aforward_listfor thetzdbdata, which (I think) isnothrowdefault-constructible, and lazy-initializing the data.file_clockhas to implement conversions with only one ofsys_timeandutc_time. I chose UTC because leap seconds can be counted going forward, but it's necessarily incomplete sinceFILETIMEdidn't count and can't represent leap seconds before 2017. If more conformant behavior is desired, it should change tosys_timeconversions. It would be source-breaking to change this later, so it should be considered carefully.w32tm, but I'm not certain how/if this can be done in the tests, since it would involve running a script as administrator.