[libcxx-commits] [PATCH] D139771: [libc++][chrono] Add hh_mm_ss formatter.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__chrono/convert_to_tm.h:129
+    // provides.
+    if constexpr (sizeof(std::chrono::hours::rep) > sizeof(__result.tm_hour))
+      if (__value.hours().count() > std::numeric_limits<decltype(__result.tm_hour)>::max())
----------------
I think this is closer to what you actually mean.


================
Comment at: libcxx/include/__chrono/formatter.h:81
   requires(chrono::__is_duration<_Tp>::value)
 _LIBCPP_HIDE_FROM_ABI void __format_sub_seconds(const _Tp& __value, basic_stringstream<_CharT>& __sstr) {
   __sstr << std::use_facet<numpunct<_CharT>>(__sstr.getloc()).decimal_point();
----------------
Same here -- we should consider using `chrono::duration<...>` here instead of `__is_duration`.


================
Comment at: libcxx/include/__chrono/formatter.h:108-109
 }
+template <class _CharT, __is_hh_mm_ss _Tp>
+_LIBCPP_HIDE_FROM_ABI void __format_sub_seconds(const _Tp& __value, basic_stringstream<_CharT>& __sstr) {
+  __sstr << std::use_facet<numpunct<_CharT>>(__sstr.getloc()).decimal_point();
----------------
Any reason for not doing this way instead?


================
Comment at: libcxx/include/__chrono/formatter.h:516
+        //
+        // TODO FMT Propbaby the wording needs to be adapted to
+        // - The displayed hours is hh_mm_ss.hours() % 24
----------------



================
Comment at: libcxx/test/std/time/time.syn/formatter.hh_mm_ss.pass.cpp:193
+#elif defined(__APPLE__)
+           "%r=''\t"
+#else
----------------
Isn't that completely broken? Do you have a short reproducer? I could probably get that fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139771