[libcxx-commits] [PATCH] D128577: [libc++][chrono] Implements formatter day.

[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/docs/Status/Cxx20Issues.csv:178
 "`3244 <https://wg21.link/LWG3244>`__","Constraints for ``Source``\  in |sect|\ [fs.path.req] insufficiently constrainty","Belfast","",""
-"`3241 <https://wg21.link/LWG3241>`__","``chrono-spec``\  grammar ambiguity in |sect|\ [time.format]","Belfast","","","|chrono| |format|"
+"`3241 <https://wg21.link/LWG3241>`__","``chrono-spec``\  grammar ambiguity in |sect|\ [time.format]","Belfast","|Complete|","15.0","|chrono| |format|"
 "`3257 <https://wg21.link/LWG3257>`__","Missing feature testing macro update from P0858","Belfast","",""
----------------



================
Comment at: libcxx/include/__chrono/formatter.h:50
+template <class _Tp>
+_LIBCPP_HIDE_FROM_ABI tm __make_tm(const _Tp& __value) {
+  tm __result;
----------------
I don't think this should be in `formatter.h` or in the `__formatter` namespace. This seems generally useful, don't you think? I might suggest something like `__chrono/convert_to_tm.h` to mirror `convert_to_timespec.h`.


================
Comment at: libcxx/include/__chrono/statically_widen.h:13
+
+// Implements the STATICALLY-WIDEN exposition only type. ([time.general]/2)
+
----------------



================
Comment at: libcxx/include/__chrono/statically_widen.h:35
+}
+#    define _LIBCPP_STATICALLY_WIDEN(_CharT, __str) std::__statically_widen<_CharT>(__str, L##__str)
+#  else // _LIBCPP_HAS_NO_WIDE_CHARACTERS
----------------
Equivalent since `_LIBCPP_STATICALLY_WIDEN` should only be used in our namespace, but still more idiomatic since this is a macro.


================
Comment at: libcxx/include/__chrono/statically_widen.h:45
+}
+#    define _LIBCPP_STATICALLY_WIDEN(_CharT, __str) std::__statically_widen<_CharT>(__str)
+#  endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS
----------------
Ditto.


================
Comment at: libcxx/include/module.modulemap.in:445
+        @requires_LIBCXX_ENABLE_LOCALIZATION@
+        @requires_LIBCXX_ENABLE_INCOMPLETE_FEATURES@
+        private header "__chrono/formatter.h"
----------------
This shouldn't be here.


================
Comment at: libcxx/include/module.modulemap.in:460
+        @requires_LIBCXX_ENABLE_LOCALIZATION@
+        @requires_LIBCXX_ENABLE_INCOMPLETE_FEATURES@
+        private header "__chrono/ostream.h"
----------------
Not needed.


================
Comment at: libcxx/include/module.modulemap.in:464
+      module parser_std_format_spec {
+        @requires_LIBCXX_ENABLE_INCOMPLETE_FEATURES@
+        private header "__chrono/parser_std_format_spec.h"
----------------
Not needed.


================
Comment at: libcxx/test/libcxx/utilities/format/format.formatter/format.formatter.spec/formattable.compile.pass.cpp:146
 
-  assert_is_not_formattable<std::chrono::day, CharT>();
+  assert_is_formattable_with_localization_support<std::chrono::day, CharT>();
   assert_is_not_formattable<std::chrono::month, CharT>();
----------------
I would prefer instead:

```
// The chrono formatters require localization support because {XXXXXXXX}
#ifndef TEST_HAS_NO_LOCALIZATION
  assert_is_formattable<std::chrono::day, CharT>();
#endif
```

We don't need to test that it's not formattable when localization is disabled.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.day/time.cal.day.nonmembers/ostream.pass.cpp:34-36
+// DEBUG
+#include <iostream>
+#include <format>
----------------
Leftover?


================
Comment at: libcxx/test/std/time/time.cal/time.cal.day/time.cal.day.nonmembers/ostream.pass.cpp:89-90
+  std::string str = std::format("-{}------------", 0d);
+  for (int i = 0; i < 5; ++i)
+    std::cerr << i << ": " << int(str[i]) << '\n';
+
----------------
Leftover?


================
Comment at: libcxx/test/std/time/time.syn/formatter_tests.h:7
+//===----------------------------------------------------------------------===//
+
+#include "make_string.h"
----------------
Missing header guard.


================
Comment at: libcxx/test/std/time/time.syn/formatter_tests.h:60-66
+#  if defined(_LIBCPP_VERSION)
+        if constexpr (std::same_as<CharT, char>)
+          if (e.what() != what)
+            std::cerr << "\nFormat string   " << fmt << "\nExpected exception " << what << "\nActual exception   "
+                      << e.what() << '\n';
+#  endif
+        LIBCPP_ASSERT(e.what() == what);
----------------



================
Comment at: libcxx/test/std/time/time.syn/formatter_tests.h:88-92
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+              std::conditional_t<std::same_as<CharT, char>, std::format_context, std::wformat_context>
+#else
+            std::format_context
+#endif
----------------
I would suggest putting that above so it doesn't clutter the body of the function so much. You could use something like `using Context = ....`. The same comment applies above too.


================
Comment at: libcxx/test/std/time/time.syn/formatter_tests.h:103-104
+#endif
+  } else
+    check_exception(what, std::basic_string_view<CharT>{fmt}, arg);
+}
----------------



================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp:119
+template <class T, class CharT>
+void assert_formatter_is_enabled_with_localization_support() {
+#ifndef TEST_HAS_NO_LOCALIZATION
----------------
Same comment as above for `TEST_HAS_NO_LOCALIZATION`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128577