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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
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())
----------------
ldionne wrote:
> I think this is closer to what you actually mean.
As mentioned in the 1:1 that part is tested on the line below, I think this looks slightly cleaner.


================
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();
----------------
ldionne wrote:
> Any reason for not doing this way instead?
Good point, not sure why I picked this solution, the patch is quite old. The followup patch doesn't require that either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139771