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

Fix leap second validation when parsing time_points #2705

Conversation

statementreply
Copy link
Contributor

  1. Fixed utc_time parsing incorrectly accepting invalid 60th second values (e.g. 00:00:60)
  2. Removed leap second validation in sys_time parsing. It now accepts 23:59:59 even if it is deleted by a negative leap second, which can be represented as a sys_time value but doesn't physically exist in UTC.
  3. Merged leap second validation and correction code to avoid looking up the leap second table twice.

Fixes #2163. Fixes #2698.

1. Fixed `utc_time` parsing incorrectly accepting invalid 60th second
   values (e.g. 00:00:60)

2. Removed leap second validation in `sys_time` parsing. It now accepts
   23:59:59 even if it is deleted by a negative leap second, which can be
   represented as a `sys_time` value but doesn't physically exist in UTC.

3. Merged leap second validation and correction code to avoid looking up
   the leap second table twice.
@statementreply statementreply requested a review from a team as a code owner May 4, 2022 04:55
@StephanTLavavej StephanTLavavej added bug Something isn't working chrono C++20 chrono labels May 4, 2022
Changes the code and comments to match the cutoff date.

Also changes date format to ISO 8601 for consistency with other
comments.
@StephanTLavavej
Copy link
Member

I've pushed a conflict-free merge with main, followed by renaming the enum class and dropping _Negative for ABI reasons (see comment). I've verified with dumpbin /symbols and undname that this affects the mangled name, which is good.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Other than the nit, this looks good to me.

@strega-nil-ms strega-nil-ms removed their assignment Jun 17, 2022
@StephanTLavavej StephanTLavavej dismissed barcharcraz’s stale review June 17, 2022 22:19

Requested changes have been made

@StephanTLavavej
Copy link
Member

I've pushed a couple of small commits to combine Nicole's clarity improvement (of handling the error case first, before we forget what we tested) with if constexpr {} else {}'s behavior of instantiating only the taken branch, and a comment update because the relative positions of the code are being reversed.

@StephanTLavavej StephanTLavavej self-assigned this Jun 19, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit d013afe into microsoft:main Jun 20, 2022
@StephanTLavavej
Copy link
Member

Thanks! That's one small step for a PR, one giant leap second for C++ <chrono>! 🧑‍🚀 ⌚ 😹

@statementreply statementreply deleted the read-system-clock-without-leap-second-check branch July 31, 2022 05:49
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Co-authored-by: Stephan T. Lavavej <[email protected]>
Co-authored-by: Nicole Mazzuca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chrono C++20 chrono
Projects
None yet
5 participants