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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for the review! I'll have a look at your other review comments after I have the results of the CI run.



================
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.
+///
----------------
vitaut wrote:
> Submit an LWG issue to clarify this?
At the moment I'm collecting the issues I've found. When the number is low I'll file LWG issues, but otherwise I probably write a paper with all issues.


================
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) {
----------------
vitaut wrote:
> Is this expensive? Maybe extract a facet lazily since it's often unused.
I haven't benchmarked it.

Interesting, I expected it to be used often since most of the formatting can be done by `__facet.put`. That saves me implementing all locale specific behaviour.

One optimization I've seen in {fmt} is that you handle the C locale as a special case and have written your own output routines for that case. I have that on my todo list.


================
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_)
----------------
vitaut wrote:
> 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.
Agreed. For now I've been mainly looking at getting things to work. But optimizing it most definitely on my todo list.


================
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.
----------------
vitaut wrote:
> Could you give an example of a format call illustrating the problem on AIX and Windows?
These are examples of the previous iteration of this patch.
Windows `check.template operator()<"{:*^23}">(SV("*** is not a valid day*"), 0d);` (the same for other invalid days.)
AIX `check.template operator()<fmt>(SV("%d='55'\t%Od='55'\t%e='55'\t%Oe='55'\n"), 255d);`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128577