[libcxx-commits] [PATCH] D137022: [libc++][chrono] Add calendar type formatters.

[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/convert_to_tm.h:43
+//
+// This is an implementation detail for the function below.
+//
----------------
Otherwise, it's too likely to become out of date.


================
Comment at: libcxx/include/__chrono/convert_to_tm.h:47
+// convert from sys_days to time_t and then to _Tm. But this leads to the Y2K
+// bug when time_t is a 32-bit signed integer. Chrono considers years beyond
+// the year 2038 valid, so instead do the transformation manually.
----------------
Do you have a test that would fail if we had that bug?

Answer: yes, it seems you ran into that bug.


================
Comment at: libcxx/include/__chrono/formatter.h:586-587
+
+  // TODO FMT is there a way to extract the index?
+  // %i as an option?
+
----------------
I would remove this from the code since it's just a personal note.


================
Comment at: libcxx/include/__chrono/ostream.h:165
+operator<<(basic_ostream<_CharT, _Traits>& __os, const month_day& __md) {
+  // TODO FMT The Standard allows 30th of Februari to be printed.
+  // It would be nice to show an error message instead.
----------------



================
Comment at: libcxx/include/atomic:2656
 
 _LIBCPP_END_NAMESPACE_STD
 
----------------
Mordante wrote:
> These are removed due to breakage see https://buildkite.com/llvm-project/libcxx-ci/builds/15514#0184ddf6-ad35-496a-8808-4b35ec626653
> 
> chrono does not affect C++03 so not listed in the release notes.
> 
~~I don't understand this. Especially since that CI run contains XPASSes, not failures. In other words, it seems like whatever change that was, it actually fixed some tests.~~

Thanks for explaining, we were mis-detecting the `non-lockfree-atomics` Lit feature.


================
Comment at: libcxx/test/std/time/time.cal/time.cal.md/time.cal.md.nonmembers/ostream.pass.cpp:77
+  assert(stream_c_locale<CharT>(std::chrono::month_day{std::chrono::month{2}, 29d}) == SV("Feb/29"));
+  // TODO FMT there is no validation in this ostream operator.
+  assert(stream_c_locale<CharT>(std::chrono::month_day{std::chrono::month{6}, 31d}) == SV("Jun/31"));
----------------
Is this a leftover?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137022