[libc++][chrono] Fixes formatter duration.

[Mordante]

Summary

@EricWF spotted this issue in the post-commit review comments of
D134742. However the suggestion to just use chrono calculations can
result in similar issues when using small fractional seconds.

[mclow.lists]

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

1169

Are you sure this number shouldn't be 65'535 (aka 0FFFF)?

Wouldn't it be better to write them all in hex, so it's clear what they're represententing?

[Mordante]

Addresses review comments.

[Mordante]

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

1169

Good catch, I've applied your suggestion.

[ldionne]

Deferring approval to @EricWF.

[hiraditya]

libcxx/include/__chrono/convert_to_tm.h

130

nit: tabs

[Mordante]

Rebased and addresses review comments.

[EricWF]

libcxx/include/__chrono/convert_to_tm.h

123

In what case can it be converted to seconds but not hours?

Also is there a test case for the case for the case where the type cannot be converted to hours?

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

1163

Can't we just use std::chrono::years::max?

Do these test cases fail before your fixes? It looks to me that they're actively avoiding constructing test values that will overflow.

[Mordante]

libcxx/include/__chrono/convert_to_tm.h

123

Is the comment unclear?

There are tests, I'll comment below.

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

43

This value can't be converted to hours.

1163

These tests test the Standard mandated limits as stated by the comment on line 1081. Using larger values will make the tests non-portable.

[Mordante]

Rebased to trigger CI.

[EricWF]

This is a better state of the world for sure. I still think there may be ways to do better, but perfect is the enemy of progress, so let's progress for now and think about perfect later.

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

1163

Right, but it's important we don't introduce UB if a user violates the limits (which they will).

[EricWF]

libcxx/include/__chrono/convert_to_tm.h

123

If it can't be converted to hours because it potentially overflows, why is it that converting to seconds works?

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

1163

I don't understand where in the standard you're finding this requirement. Could you please provide the stable name?

[Mordante]

In D138826#4487269, @EricWF wrote:

This is a better state of the world for sure. I still think there may be ways to do better, but perfect is the enemy of progress, so let's progress for now and think about perfect later.

Thanks for the review! I will give this approach some more thought. But I too prefer to get this fix in.

libcxx/include/__chrono/convert_to_tm.h

123

Since our implementation uses the same number of bits for hours and seconds.

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

1163

It's in [time.syn] http://eel.is/c++draft/time.syn
I've added that to the comment on line 1111.

libcxx/include/__chrono/convert_to_tm.h

 #include <__chrono/year.h>
 #include <__chrono/year_month.h>
 #include <__chrono/year_month_day.h>
 #include <__chrono/year_month_weekday.h>
 #include <__concepts/same_as.h>
 #include <__config>
 #include <__format/format_error.h>
 #include <__memory/addressof.h>
+#include <__type_traits/is_convertible.h>
 #include <cstdint>
 #include <ctime>
 #include <limits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
else if constexpr (same_as<typename _ChronoT::clock, chrono::local_t>)
       return std::__convert_to_tm<_Tm>(chrono::sys_time<typename _ChronoT::duration>{__value.time_since_epoch()});
     else
       static_assert(sizeof(_ChronoT) == 0, "TODO: Add the missing clock specialization");
   } else if constexpr (chrono::__is_duration<_ChronoT>::value) {
     // [time.format]/6
     //   ...  However, if a flag refers to a "time of day" (e.g. %H, %I, %p,
     //   etc.), then a specialization of duration is interpreted as the time of
     //   day elapsed since midnight.
+
+    // Not all values can be converted to hours, it may run into ratio
+    // conversion errors. In that case the conversion to seconds works.
+    if constexpr (is_convertible_v<_ChronoT, chrono::hours>) {
+      auto __hour      = chrono::floor<chrono::hours>(__value);
+      auto __sec       = chrono::duration_cast<chrono::seconds>(__value - __hour);
+      __result.tm_hour = __hour.count() % 24;
+      __result.tm_min  = __sec.count() / 60;
+      __result.tm_sec  = __sec.count() % 60;
+    } else {
       uint64_t __sec = chrono::duration_cast<chrono::seconds>(__value).count();
       __sec %= 24 * 3600;
       __result.tm_hour = __sec / 3600;
       __sec %= 3600;
       __result.tm_min = __sec / 60;
       __result.tm_sec = __sec % 60;
+    }
   } else if constexpr (same_as<_ChronoT, chrono::day>)
     __result.tm_mday = static_cast<unsigned>(__value);
   else if constexpr (same_as<_ChronoT, chrono::month>)
     __result.tm_mon = static_cast<unsigned>(__value) - 1;
   else if constexpr (same_as<_ChronoT, chrono::year>)
     __result.tm_year = static_cast<int>(__value) - 1900;
   else if constexpr (same_as<_ChronoT, chrono::weekday>)
     __result.tm_wday = __value.c_encoding();

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp

 #include "make_string.h"
 #include "platform_support.h" // locale name macros
 #include "test_macros.h"
 
 template <class CharT>
 static void test_no_chrono_specs() {
   using namespace std::literals::chrono_literals;
 
   check(SV("1as"), SV("{}"), std::chrono::duration<int, std::atto>(1));
   check(SV("1fs"), SV("{}"), std::chrono::duration<int, std::femto>(1));
   check(SV("1ps"), SV("{}"), std::chrono::duration<int, std::pico>(1));
   check(SV("1ns"), SV("{}"), 1ns);
 #ifndef TEST_HAS_NO_UNICODE
   check(SV("1\u00b5s"), SV("{}"), 1us);
 #else
   check(SV("1us"), SV("{}"), 1us);
 #endif
check_invalid_types<CharT>(
       0ms);
 
   check_exception("Expected '%' or '}' in the chrono format-string", SV("{:A"), 0ms);
   check_exception("The chrono-specs contains a '{'", SV("{:%%{"), 0ms);
   check_exception("End of input while parsing the modifier chrono conversion-spec", SV("{:%"), 0ms);
   check_exception("End of input while parsing the modifier E", SV("{:%E"), 0ms);
   check_exception("End of input while parsing the modifier O", SV("{:%O"), 0ms);
 
+  // Make sure the the required values work, based on their minimum number of required bits per [time.syn].
+  check(SV("23:47:16.854775807"),
+        SV("{:%T}"),
+        std::chrono::nanoseconds{0x7fff'ffff'ffff'ffffll}); // 64 bit signed value max
+  check(SV("23:35:09.481983"),
+        SV("{:%T}"),
+        std::chrono::microseconds{0x003f'ffff'ffff'ffffll});                                 // 55 bit signed value max
+  check(SV("06:20:44.415"), SV("{:%T}"), std::chrono::milliseconds{0x0000'fff'ffff'ffffll}); // 45 bit signed value max
+  check(SV("01:53:03"), SV("{:%T}"), std::chrono::seconds{0x0000'0003'ffff'ffffll});         // 35 bit signed value max
+  check(SV("12:15:00"), SV("{:%T}"), std::chrono::minutes{0x0fff'ffff});                     // 29 bit signed value max
+  check(SV("15:00:00"), SV("{:%T}"), std::chrono::hours{0x003f'ffff});                       // 23 bit signed value max
+  check(SV("00:00:00"), SV("{:%T}"), std::chrono::days{0x0ff'ffff});                         // 25 bit signed value max
+  check(SV("00:00:00"), SV("{:%T}"), std::chrono::weeks{0x003f'ffff});                       // 22 bit signed value max
+  check(SV("21:11:42"), SV("{:%T}"), std::chrono::months{0x0007'ffff});                      // 20 bit signed value max
+  check(SV("05:42:00"), SV("{:%T}"), std::chrono::years{0xffff});                            // 17 bit signed value max
+
   // Precision not allowed
   check_exception("Expected '%' or '}' in the chrono format-string", SV("{:.3}"), 0ms);
 }
 
 int main(int, char**) {
   test<char>();
 
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test<wchar_t>();
 #endif
 
   return 0;
 }