This is an archive of the discontinued LLVM Phabricator instance.

[libc++][chrono] Add hh_mm_ss formatter.
ClosedPublic

Authored by Mordante on Dec 10 2022, 3:30 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG7f5d130a428f: [libc++][chrono] Add hh_mm_ss formatter.
Summary

Partially implements:

  • P1361 Integration of chrono with text formatting
  • P2372 Fixing locale handling in chrono formatters
  • P1466 Miscellaneous minor fixes for chrono

Depends on D137022

Diff Detail

Event Timeline

Mordante created this revision.Dec 10 2022, 3:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2022, 3:30 AM
Mordante updated this revision to Diff 481843.Dec 10 2022, 4:21 AM

CI fixes.

Mordante updated this revision to Diff 481846.Dec 10 2022, 4:59 AM

CI fixes.

Mordante updated this revision to Diff 481849.Dec 10 2022, 5:36 AM

CI fixes.

Mordante updated this revision to Diff 481855.Dec 10 2022, 7:02 AM

Enable entire CI.

Mordante updated this revision to Diff 489616.Jan 16 2023, 11:41 AM

Rebased and some minor improvements.

Mordante updated this revision to Diff 489835.Jan 17 2023, 8:22 AM

Adds missing includes.

Mordante updated this revision to Diff 489863.Jan 17 2023, 9:41 AM

CI fixes.

Mordante updated this revision to Diff 491188.Jan 22 2023, 10:21 AM

Rebased to test CI.

Mordante updated this revision to Diff 491196.Jan 22 2023, 12:06 PM

Disable GCC tests.

Mordante updated this revision to Diff 491405.Jan 23 2023, 8:57 AM

Reviewed and CI fixes.

Mordante published this revision for review.Jan 24 2023, 8:21 AM
Mordante added reviewers: ldionne, vitaut.
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 8:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
h-vetinari added inline comments.
libcxx/include/__chrono/formatter.h
522–523

For leap seconds, it would be legal to have a duration of a day take 24:00:01.

Mordante added inline comments.Jan 25 2023, 10:00 AM
libcxx/include/__chrono/formatter.h
522–523

But that should be displayed as 23:59:60, which probably doesn't work either. The formatting is done using a tm struct, there 24 is not a valid value for tm_hour https://en.cppreference.com/w/cpp/chrono/c/tm.

Looking at http://eel.is/c++draft/time.hms#members-3 a duration of 24*3600 + 1 seconds will be initialized to 24 hours and one second; but without any calendar information and a leapseconds database it's unknown whether it's a leap second or not.

Also see the comment above; it's a bit underspecified what the code should do. Originally this class was a time of day, but that has been changed.
Note this issue exception will only be thrown when you try to format the time using a 12 or 24 hour clock. Formatting the hours itself will work for values > 23.

ldionne requested changes to this revision.Jan 31 2023, 10:08 AM
ldionne added inline comments.
libcxx/include/__chrono/convert_to_tm.h
124

I think this is closer to what you actually mean.

libcxx/include/__chrono/formatter.h
81

Same here -- we should consider using chrono::duration<...> here instead of __is_duration.

108–109

Any reason for not doing this way instead?

516
libcxx/test/std/time/time.syn/formatter.hh_mm_ss.pass.cpp
194

Isn't that completely broken? Do you have a short reproducer? I could probably get that fixed.

This revision now requires changes to proceed.Jan 31 2023, 10:08 AM
Mordante marked 5 inline comments as done.Feb 5 2023, 11:38 PM

Thanks for the review!

libcxx/include/__chrono/convert_to_tm.h
124

As mentioned in the 1:1 that part is tested on the line below, I think this looks slightly cleaner.

libcxx/include/__chrono/formatter.h
108–109

Good point, not sure why I picked this solution, the patch is quite old. The followup patch doesn't require that either.

Mordante updated this revision to Diff 495008.Feb 5 2023, 11:38 PM
Mordante marked 2 inline comments as done.

Addresses review comments.

ldionne accepted this revision.Feb 14 2023, 9:01 AM
This revision is now accepted and ready to land.Feb 14 2023, 9:01 AM
This revision was landed with ongoing or failed builds.Feb 14 2023, 10:12 AM
This revision was automatically updated to reflect the committed changes.