Skip to content

Conversation

Saalvage
Copy link
Contributor

@Saalvage Saalvage commented May 21, 2023

Summary of changes:

  • Remove explicit assertion that prevents formatting hh_mm_sss >= 1 day
  • Change hh_mm_ss' formatting implementation to use the hours and minutes explicitly (put_time does not deal with hours >= 24, which is desired behavior)
  • Remove _Hh_mm_ss_part_underflow_to_zero and instead cast every duration to be formatted this way to hh_mm_ss directly (this is the format("{:%T}", some_duration) == "00:00:00" bug discussed in the original issue (not the same bug, but closely related ;))
  • Simplified abs for durations slightly.
  • Added custom %H format override.

Fixes #3676

@Saalvage Saalvage requested a review from a team as a code owner May 21, 2023 23:48
@CaseyCarter CaseyCarter added bug Something isn't working chrono C++20 chrono format C++20/23 format labels May 24, 2023
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

Thanks! I've pushed changes to fix hh_mm_ss's constructor (essentially going back to the main implementation, although I'm adding _Remove_duration_part<_CHRONO hours> to more closely follow the Standard) and fully update the expected results for 1.55f days, which allows all of the tests to pass.

@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 99d36f5 into microsoft:main Jun 15, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing this runtime correctness bug - oh happy day! 😹 📅 🎉

@Saalvage Saalvage deleted the fix/hh_mm_ss-large-values-format branch June 15, 2023 15:00
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 format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<chrono>: Formatting large hh_mm_ss values should be permitted
5 participants