[libcxx-commits] [PATCH] D138826: [libc++][chrono] Fixes formatter duration.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Apr 14 09:01:49 PDT 2023
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
More information about the libcxx-commits
mailing list