[libcxx-commits] [PATCH] D133663: [libc++][chrono] Implements formatter year.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: libcxx/include/__chrono/formatter.h:62-64
+_LIBCPP_HIDE_FROM_ABI void __format_year_last_two_digits(int __year, basic_stringstream<_CharT>& __sstr) {
+  __sstr << std::format(_LIBCPP_STATICALLY_WIDEN(_CharT, "{:02}"), (std::abs(__year + 1900)) % 100);
+}
----------------
Since this is used in a single place, any reason not to inline it where it's used?


================
Comment at: libcxx/include/__chrono/formatter.h:69
+_LIBCPP_HIDE_FROM_ABI void __format_year(int __year, basic_stringstream<_CharT>& __sstr) {
+  __year += 1900; // tm_year to year conversion.
+  if (__year < 0) {
----------------
Let's perform the `tm` => `int-from-year-0` conversion in the caller of this function instead. Applies above and below too.


================
Comment at: libcxx/include/__chrono/formatter.h:72
+    __sstr << _CharT('-');
+    // The chrono library requires a year fits in a short.
+    __year = -__year;
----------------
I don't understand this comment.


================
Comment at: libcxx/include/__chrono/formatter.h:81
+
+  // Note accoriding to the wording it should be left padded, which is odd.
+  __sstr << std::format(_LIBCPP_STATICALLY_WIDEN(_CharT, "{:04}"), __year);
----------------



================
Comment at: libcxx/include/__chrono/formatter.h:137-140
+        // Japanese EY contains the era name and year. This is zero padded to 2
+        // digits time_put (note older glibc versions didn't do padding.)
+        // However most eras won't reach a 100 years, let alone 1000. So
+        // padding to 4 digits seems unwanted for Japanese.
----------------



================
Comment at: libcxx/test/libcxx/transitive_includes/cxx20.csv:79
 chrono limits
+chrono locale
 chrono ratio
----------------
This should go away once you rebase onto `main` with the updated transitive includes tests.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.year/time.cal.year.nonmembers/ostream.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-has-no-localization
+// UNSUPPORTED: libcpp-has-no-incomplete-format
----------------
Here and in the other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133663