[libcxx-commits] [PATCH] D134138: [libc++][chrono] Implements formatter month.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__chrono/formatter.h:188
+template <class _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr bool __month_name_ok(const _Tp& __value) {
+  if constexpr (same_as<_Tp, chrono::day>)
----------------
For watchers, this looks quite weird right now but Mark explained that this was going to be extended for other values, and the logic will become non-trivial. So I think this is mandated indeed.


================
Comment at: libcxx/include/__chrono/formatter.h:220
+    if (__specs.__chrono_.__month_name_ && !__formatter::__month_name_ok(__value))
+      std::__throw_format_error("formatting a month name needs a valid value");
+
----------------



================
Comment at: libcxx/include/__chrono/ostream.h:51
+                                         _LIBCPP_STATICALLY_WIDEN(_CharT, "{} is not a valid month"),
+                                         static_cast<unsigned>(__m))); // TODO FMT locale not needed
+}
----------------
Can you somehow indicate that this is likely a bug in the spec, not in our implementation? As currently worded, the comment seems to imply that we're doing something wrong.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.month/time.cal.month.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 elsewhere.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.month/time.cal.month.nonmembers/ostream.pass.cpp:16-17
+
+// REQUIRES: locale.fr_FR.UTF-8
+// REQUIRES: locale.ja_JP.UTF-8
+
----------------
Can you double-check that these tests are getting run at all in our CI? It seems rather suspicious to me that the no-localization CI job didn't fail despite using the incorrect `// UNSUPPORTED: libcpp-has-no-localization` formulation above.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.month/time.cal.month.nonmembers/ostream.pass.cpp:19-20
+
+// <chrono>
+// class month;
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134138