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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 8 inline comments as done.
Mordante 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)
----------------
ldionne wrote:
> 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.
I think moving has another advantage. All other preprocessor are no longer in a global one, thus decreasing their indention level.
(This avoid reformatting when the guard is removed.)


================
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
----------------
ldionne wrote:
> Instead, I think our validation for `#pragma GCC system_header` should be less strict.
Agreed I'll look at that in a separate commit.


================
Comment at: libcxx/include/__chrono/formatter.h:60-61
+  tm __result{
+      .tm_sec   = 0,
+      .tm_min   = 0,
+      .tm_hour  = 0,
----------------
ldionne wrote:
> 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.
I haven't found a clang-format option to do this. Only for the constructor. But I agree it would be great when clang-format would have an option to do this.


================
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"));
----------------
ldionne wrote:
> Why are we checking `255` if it's not a valid input? (and hence has these various weird results on AIX and other platforms)
As discussed in our private conversation. `255` is the minimum of the maximum supported value for a day. I'm not happy with how this works in practice and want to look at alternative solutions. (I will most likely file an LWG issue or write a paper.)

For now I leave it as is since I want to look closer are when exactly happens on Windows; however I want to have a better solution before landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128577