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

chrono cleanups #1779

Merged

Conversation

StephanTLavavej
Copy link
Member

<chrono>

  • Change insert()-at-begin() to assign() which is strictly more efficient. This is for a freshly constructed vector where only reserve() has been called, and this isn't in a loop, so the behavior is unchanged. (Note that assign() preserves the reserved capacity when it's sufficient, which is the case here.)
  • Remove // TRANSITION: work in progress from tzdb_list (which is complete AFAIK).
  • Comment why get_tzdb_list() throws runtime_error instead of bad_alloc for allocation failure.
  • Enhance get_tzdb_list()'s exception handling:
    • Use _TRY_BEGIN etc. to preserve our minimal support for _HAS_EXCEPTIONS=0. (We need to directly test #if _HAS_EXCEPTIONS when referring to _Except, because it won't be declared in _HAS_EXCEPTIONS=0 mode.)
    • Add a case to rethrow runtime_error unchanged.
    • In both runtime_error and exception cases, we need to __std_free_crt in order to avoid leaking.
  • Similarly, enhance reload_tzdb()'s EH.
  • Add _CHRONO qualification to non-_Ugly function calls.
  • Enhance tzdb_list's locking. The Standardese talks about multithreading for specific functions or pairs of functions (WG21-N4885 [time.zone.db.list]/3, front() "is thread-safe with respect to reload_tzdb()", [time.zone.db.access]/2 get_tzdb_list() "It is safe to call this function from multiple threads at one time."), and the original implementation appeared to follow this strictly, but I propose to extend this by applying reader/writer locks to all member functions. (Note, for example, that if another thread is simultaneously reloading to emplace_front a new element, neither front() nor begin() can be called simultaneously. The Standard says that front() must be thread-safe in this scenario, so we took a _Shared_lock, but it's silent about begin() which really seems like a defect to me.
    • There are naturally some remaining restrictions (e.g. can't erase_after() while someone is iterating through), but this seems like a cheap way to prevent rare catastrophes.
    • _Shared_lock here is never manually _Unlock()ed, so we can drop that and its bool _Owns to be slightly more efficient.

xtzdb.h, src/tzdb.cpp

  • Mark __std_tzdb_get_reg_leap_seconds() as _NODISCARD for consistency and because it allocates.

tests/std/include/timezone_data.hpp

  • <string> appeared to be completely unused.

P0355R7_calendars_and_time_zones_clocks/test.cpp

  • Include <algorithm> for find().
  • Directly construct const sys_days ls{ymd};, avoiding "Almost Always Auto".
  • Rename ugly _Tz in test code.

P0355R7_calendars_and_time_zones_io/test.cpp

  • As this is C++20, we can use pair CTAD instead of make_pair(), which is simpler (and slightly less debug codegen, not that it matters in test code).

P0355R7_calendars_and_time_zones_time_zones/test.cpp

  • Fix comment typos.

@StephanTLavavej StephanTLavavej added the chrono C++20 chrono label Mar 25, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner March 25, 2021 22:49
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Thanks for these cleanups! Looks good to me 😎

@mnatsuhara
Copy link
Contributor

Looking through some of the PRs that merged while I was OOF, I'm wondering if we can roll any of these old comments into this PR as well:

I also suggest removing the references to "reg"/"registry" in the naming of the __std_tzdb_* functions since (1) the source of information is not relevant outside of the DLL and (2) there is some discussion over potentially changing the leap second approach to get this information elsewhere, in which case the "reg"/"registry" in names would be confusing (see comment).

Copy link
Contributor

@d-winsor d-winsor left a comment

Choose a reason for hiding this comment

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

These changes look good to me. Might be able to roll in the removal of a temporary in tzdb_list insertion. I didn't get around to fixing this in previous PRs.

_Tzdb_list.emplace_front(tzdb{

_Tzdb_list.emplace_front(tzdb{

@StephanTLavavej
Copy link
Member Author

Might be able to roll in the removal of a temporary in tzdb_list insertion.

I was confused when I asked for that before - tzdb is an aggregate, so I believe we can't emplace_back it directly.

@StephanTLavavej
Copy link
Member Author

I've pushed additional changes:

  • Merged feature/chrono to avoid conflicts.
  • Override clang-format for zoned_time.
    • AlignConsecutiveAssignments was causing trouble with the = for default template arguments.
  • Rename <xtzdb.h> to <__msvc_tzdb.hpp>.
    • Also renamed the idempotency guard macro.
  • Rename _Xtzdb_meow to _Tzdb_meow.
    • For consistency with the header rename. Also, _Xmeow functions are conventionally exception-throwing helpers (there are only a few exceptions).
  • Rename __std_tzdb_registry_leap_info to __std_tzdb_leap_info.
    • Also renamed an _Rlsi parameter to _Info.
    • Note that local variables, comments, etc. still mention the registry - the implementation isn't changing, we just want to make the interface independent of the implementation.
  • Rename to __std_tzdb_get_leap_seconds/__std_tzdb_delete_leap_seconds.
    • Also renamed parameters in the declaration, but not the definition (they already differed in _Ugliness).
  • Changed FIXME comment to cite <chrono>: Should divergence between ICU and IANA be reported? #1786 for followup.

Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks 🕐

@StephanTLavavej StephanTLavavej merged commit 3017b91 into microsoft:feature/chrono Mar 26, 2021
@StephanTLavavej StephanTLavavej deleted the review_chrono branch March 26, 2021 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrono C++20 chrono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants