[libcxx-commits] [PATCH] D139771: [libc++][chrono] Add hh_mm_ss formatter.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__chrono/formatter.h:522-523
+        //   - or write the number of days.
+        if (__specs.__chrono_.__hour_ && __value.hours().count() > 23)
+          std::__throw_format_error("formatting a hour needs a valid value");
+
----------------
h-vetinari wrote:
> For [[ https://en.wikipedia.org/wiki/Leap_second | leap seconds ]], it would be legal to have a duration of a day take `24:00:01`.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139771/new/

https://reviews.llvm.org/D139771