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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__chrono/convert_to_tm.h:88
+    // 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);
----------------
EricWF wrote:
> 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?
Is the comment unclear?

There are tests, I'll comment below.


================
Comment at: libcxx/test/std/time/time.syn/formatter.duration.pass.cpp:43
 
   check(SV("1as"), SV("{}"), std::chrono::duration<int, std::atto>(1));
   check(SV("1fs"), SV("{}"), std::chrono::duration<int, std::femto>(1));
----------------
This value can't be converted to hours.


================
Comment at: libcxx/test/std/time/time.syn/formatter.duration.pass.cpp:1089
+  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
----------------
EricWF wrote:
> 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.
These tests test the Standard mandated limits as stated by the comment on line 1081. Using larger values will make the tests non-portable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138826