[libcxx-commits] [PATCH] D138826: [libc++][chrono] Fixes formatter duration.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante created this revision.
Mordante added a reviewer: EricWF.
Herald added a project: All.
Mordante requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

@EricWF spotted this issue in the post-commit review comments of
D134742 <https://reviews.llvm.org/D134742>. However the suggestion to just use chrono calculations can
result in similar issues when using small fractional seconds.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138826

Files:
  libcxx/include/__chrono/convert_to_tm.h
  libcxx/test/std/time/time.syn/formatter.duration.pass.cpp


Index: libcxx/test/std/time/time.syn/formatter.duration.pass.cpp
===================================================================
--- libcxx/test/std/time/time.syn/formatter.duration.pass.cpp
+++ libcxx/test/std/time/time.syn/formatter.duration.pass.cpp
@@ -550,7 +550,7 @@
            "\n"),
         lfmt,
         std::chrono::duration<int, std::ratio<86400>>(7));
-#else // defined(__APPLE__) || defined(_AIX)
+#else  // defined(__APPLE__) || defined(_AIX)
   check(loc,
         SV("%H='00'\t"
            "%OH='〇'\t"
@@ -1076,6 +1076,22 @@
   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
+  check(SV("23:47:16.854775807"),
+        SV("{:%T}"),
+        std::chrono::nanoseconds{9'223'372'036'854'775'807ll}); // 64 bit signed value max
+  check(SV("23:35:09.481983"),
+        SV("{:%T}"),
+        std::chrono::microseconds{18'014'398'509'481'983ll});                              // 55 bit signed value max
+  check(SV("06:20:44.415"), SV("{:%T}"), std::chrono::milliseconds{17'592'186'044'415ll}); // 45 bit signed value max
+  check(SV("01:53:04"), SV("{:%T}"), std::chrono::seconds{17'179'869'184ll});              // 35 bit signed value max
+  check(SV("12:15:00"), SV("{:%T}"), std::chrono::minutes{268'435'455});                   // 29 bit signed value max
+  check(SV("15:00:00"), SV("{:%T}"), std::chrono::hours{4'194'303});                       // 23 bit signed value max
+  check(SV("00:00:00"), SV("{:%T}"), std::chrono::days{16'777'215});                       // 25 bit signed value max
+  check(SV("00:00:00"), SV("{:%T}"), std::chrono::weeks{2'097'151});                       // 22 bit signed value max
+  check(SV("15:11:42"), SV("{:%T}"), std::chrono::months{542'287});                        // 20 bit signed value max
+  check(SV("12:18:00"), SV("{:%T}"), std::chrono::years{65'565});                          // 17 bit signed value max
+
   // Precision not allowed
   check_exception("Expected '%' or '}' in the chrono format-string", SV("{:.3}"), 0ms);
 }
Index: libcxx/include/__chrono/convert_to_tm.h
===================================================================
--- libcxx/include/__chrono/convert_to_tm.h
+++ libcxx/include/__chrono/convert_to_tm.h
@@ -17,6 +17,7 @@
 #include <__chrono/year.h>
 #include <__concepts/same_as.h>
 #include <__config>
+#include <__type_traits/is_convertible.h>
 #include <cstdint>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -41,12 +42,23 @@
     //   ...  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.
-    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;
+
+    // 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>)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138826.478254.patch
Type: text/x-patch
Size: 4006 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20221128/47c167d1/attachment.bin>