Loading time_zone and time_zone_link names
#1723
Conversation
mnatsuhara
left a comment
mnatsuhara
left a comment
There was a problem hiding this comment.
I'v reviewed everything except the big stuff (<chrono>, and tests) - the ICU / Windows-y stuff takes me a bit longer to wrap my head around, so that's what I focused on today.
mnatsuhara
left a comment
mnatsuhara
left a comment
There was a problem hiding this comment.
Reviewed <chrono> -- next up: tests!
stl/inc/chrono
Outdated
| _Tzdb_list.emplace_front( | ||
| tzdb{_STD move(_Time_zones), _STD move(_Links), _STD move(_Leap_sec), _All_ls_positive}); |
There was a problem hiding this comment.
What is the incompatibility (probably error?) that you're seeing? I don't think __cpp_lib_constexpr_dynamic_alloc would be relevant here since these aren't constexpry classes (the only constexpred things in all of [time.zone] are in leap_second and local_info). std::construct_at generally speaking should be equivalent to a placement new (except it's blessed by the constexpr fairy)-- if we have differing compiler behavior here (OK for cl, not OK for clang-cl and EDG) then we need to determine who is right and potentially file some bugs 🐞
mnatsuhara
left a comment
mnatsuhara
left a comment
There was a problem hiding this comment.
Approving with suggestions! Only one comment is actually actionable -- we should decide whether we want to address the versioning question in this PR or if we should make a card for the project and fix it in a follow up PR (I'm leaning toward this option in the interest of expediency).
| _Tzdb_list.emplace_front( | ||
| tzdb{_Tzdb.version, _STD move(_Zones), _STD move(_Links), _STD move(_Leap_sec), _All_ls_positive}); |
There was a problem hiding this comment.
Since this is the case when we are actually updating information in the tzdb (because there are new leap_seconds to keep track of) we should change the version of the tzdb before emplacing it back (since things like reload_tzdb() and remote_version() use the version to differentiate between different information they contain). Right now we're getting the version from ICU, but I don't think that'll be sufficient because we'll also need to factor in leap seconds data. We need to come up with a better versioning system here...
I don't want this to hold up this PR because there is other work built on top of this, so I'm also fine with filing a card in the chrono project to revisit this in a follow-up PR.
There was a problem hiding this comment.
sounds good to me, lets revisit this
tests/std/tests/P0355R7_calendars_and_time_zones_clocks/test.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>
5 participants
Works toward #12
This is part 1 of an implementation of time_zones that retrieves IANA database information by dynamically loading the icu.dll available in certain builds of windows.
This PR handles all the infrastructure of consuming ICU Apis and loading the
time_zone&time_zone_linknames. Part 2 will build on everything here to handle the time_zone transitions and sys_time to local_time. The content of Part 2 is mostly straightforward but verbose so I left to simply review.I have scattered
// FIXME:comments in the code where I either unsure of the right approach or would like to discuss design limitations of using the ICU API surface. The long list of names in the tests will be removed and exist only to demonstrate limitations.