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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/include/__chrono/formatter.h:36
+/// 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
----------------
module -> modulo


================
Comment at: libcxx/include/__chrono/formatter.h:40-41
+///
+/// The Standard doesn't specify what to do in this case so the result depends
+/// on the result of the underlying code.
+///
----------------
Submit an LWG issue to clarify this?


================
Comment at: libcxx/include/__chrono/formatter.h:51
+_LIBCPP_HIDE_FROM_ABI tm __make_tm(const _Tp& __value) {
+  tm __result{
+      .tm_sec   = 0,
----------------
You could make this more robust and shorter:

```
tm __result = tm();
#ifdef __GLIBC__
__result.tm_zone = "UTC";
#endif
```



================
Comment at: libcxx/include/__chrono/formatter.h:79
+  tm __t              = __formatter::__make_tm(__value);
+  const auto& __facet = std::use_facet<time_put<_CharT>>(__sstr.getloc());
+  for (auto __it = __chrono_specs.begin(); __it != __chrono_specs.end(); ++__it) {
----------------
Is this expensive? Maybe extract a facet lazily since it's often unused.


================
Comment at: libcxx/include/__chrono/formatter.h:116-130
+  basic_stringstream<_CharT> __sstr;
+  // [time.format]/2
+  // 2.1 - the "C" locale if the L option is not present in chrono-format-spec, otherwise
+  // 2.2 - the locale passed to the formatting function if any, otherwise
+  // 2.3 - the global locale.
+  // Note that the __ctx's locale() call does 2.2 and 2.3.
+  if (__specs.__chrono_.__locale_specific_form_)
----------------
Not necessarily in the current diff but you might want to refactor this to only use iostreams for localized formatting. Nonlocalized can be implemented much more efficiently.


================
Comment at: libcxx/include/__chrono/ostream.h:35-39
+              // Note this error differs from the wording of the Standard. The
+              // Standard wording doesn't work well on AIX or Windows. There
+              // the formatted day seems to be either modulo 100 or completely
+              // omitted. Judging by the wording this is valid.
+              // TODO FMT Write a paper of file an LWG issue.
----------------
Could you give an example of a format call illustrating the problem on AIX and Windows?


================
Comment at: libcxx/include/__chrono/parser_std_format_spec.h:85
+  if ((__flags & __flags::__hour) != __flags::__hour)
+    std::__throw_format_error("The supplied date time doesn't contain a hour");
+}
----------------
a -> an


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128577