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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__chrono/formatter.h:15
+#include <version>
+// Enable the contents of the header only when libc++ was built with LIBCXX_ENABLE_INCOMPLETE_FEATURES.
+#if !defined(_LIBCPP_HAS_NO_INCOMPLETE_FORMAT)
----------------
I think it's fine to guard the header here, but I would remove the comment, which seems a bit redundant.

The usual tradition in libc++ is to guard the header itself (as done here) so that the header can always be included (although it might be empty). Note that this isn't really done consistently everywhere, for example `_LIBCPP_HAS_NO_LOCALIZATION` is usually checked before including the header, since the header will `#error` out if included.

So yeah, bottom line: you can keep as-is, and I guess we don't have a ton of consistency here in the library.


================
Comment at: libcxx/include/__chrono/formatter.h:27-31
+// clang-format off
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+// clang-format on
----------------
Instead, I think our validation for `#pragma GCC system_header` should be less strict.


================
Comment at: libcxx/include/__chrono/formatter.h:43
+/// the value is outside the valid range it's unspecified what strftime will
+/// output. For example weekday 8, can print 1 when the day is processed module
+/// 7 since that handles the Sunday for 0-based weekday. It can also print 8 if
----------------



================
Comment at: libcxx/include/__chrono/formatter.h:48
+/// The Standard doesn't specify what to do in this case so the result depends
+/// on the result of the underlaying code.
+///
----------------



================
Comment at: libcxx/include/__chrono/formatter.h:53
+///      of Table 97: Meaning of conversion specifiers [tab:time.format.spec].
+/// Formats a time based on a tm struct.
+///
----------------
This seems like a leftover.


================
Comment at: libcxx/include/__chrono/formatter.h:60-61
+  tm __result{
+      .tm_sec   = 0,
+      .tm_min   = 0,
+      .tm_hour  = 0,
----------------
Suggestion:

```
  .tm_sec = 0
, .tm_min = 0
, .tm_hour = 0
...
#ifdef __GLIBC__
, .tm_gmtoff = 0
, .tm_zone = "UTC"
#endif
```

IDK if clang-format will handle that gracefully.


================
Comment at: libcxx/include/__chrono/formatter.h:86
+    const _Tp& __value, basic_stringstream<_CharT>& __sstr, basic_string_view<_CharT> __chrono_specs) {
+  tm __t              = __make_tm(__value);
+  const auto& __facet = std::use_facet<time_put<_CharT>>(__sstr.getloc());
----------------
ADL


================
Comment at: libcxx/include/__chrono/statically_widen.h:33
+}
+#    define _STATICALLY_WIDEN(__str) std::__statically_widen<_CharT>(__str, L##__str)
+#  else // _LIBCPP_HAS_NO_WIDE_CHARACTERS
----------------
This should be called `_LIBCPP_STATICALLY_WIDEN`. We don't define macros that are not prefix with `_LIBCPP` (except `_NOEXCEPT` and `_NOEXCEPT_`, and IMO those were mistakes if only for consistency).

Also, we shouldn't be using `_CharT` in this macro without taking it as an argument. This creates a really weird dependency that the macro can only be used within a template with a suitably named template parameter.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.day/time.cal.day.nonmembers/ostream.pass.cpp:85
+  assert(stream_fr_FR_locale<CharT>(31d) == SV("31"));
+#ifdef _AIX
+  assert(stream_fr_FR_locale<CharT>(255d) == SV("55 is not a valid day"));
----------------
Why are we checking `255` if it's not a valid input? (and hence has these various weird results on AIX and other platforms)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128577