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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

In D138826#4487269 <https://reviews.llvm.org/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.



================
Comment at: libcxx/include/__chrono/convert_to_tm.h:123
+    // 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:
> If it can't be converted to hours because it potentially overflows, why is it that converting to seconds works?
> 
> 
Since our implementation uses the same number of bits for hours and seconds.


================
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:
> EricWF wrote:
> > Mordante wrote:
> > > 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.
> > Right, but it's important we don't introduce UB if a user violates the limits (which they will). 
> I don't understand where in the standard you're finding this requirement. Could you please provide the stable name?
It's in `[time.syn]` http://eel.is/c++draft/time.syn
I've added that to the comment on line 1111.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138826