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
Details
Details
- Reviewers
ldionne vitaut - Group Reviewers
Restricted Project - Commits
- rG7f5d130a428f: [libc++][chrono] Add hh_mm_ss formatter.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__chrono/formatter.h | ||
|---|---|---|
| 526–527 | For leap seconds, it would be legal to have a duration of a day take 24:00:01. | |
| libcxx/include/__chrono/formatter.h | ||
|---|---|---|
| 526–527 | 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. | |
| libcxx/include/__chrono/convert_to_tm.h | ||
|---|---|---|
| 129 | I think this is closer to what you actually mean. | |
| libcxx/include/__chrono/formatter.h | ||
| 79–81 | Same here -- we should consider using chrono::duration<...> here instead of __is_duration. | |
| 126–127 | Any reason for not doing this way instead? | |
| 520 | ||
| 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. | |
Comment Actions
Thanks for the review!
| libcxx/include/__chrono/convert_to_tm.h | ||
|---|---|---|
| 129 | 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 | ||
| 126–127 | Good point, not sure why I picked this solution, the patch is quite old. The followup patch doesn't require that either. | |