Page MenuHomePhabricator

[libc++][chrono] Implements formatter day.
Needs ReviewPublic

Authored by Mordante on Jun 25 2022, 2:39 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Summary

This implements the enabled specializaton
template<class charT> struct formatter<chrono::day, charT>;

and
template<class charT, class traits>

basic_ostream<charT, traits>&
  operator<<(basic_ostream<charT, traits>& os, const day& d);

Implements:

  • LWG 3241 chrono-spec grammar ambiguity in §[time.format]

Partially implements:

  • P1361 Integration of chrono with text formatting

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mordante updated this revision to Diff 441970.Jul 3 2022, 11:59 AM

More fixes.

Mordante planned changes to this revision.Jul 3 2022, 12:23 PM

Needs additional work to get a good result for invalid values.

Mordante updated this revision to Diff 445230.Jul 16 2022, 6:03 AM

Use alternative approach for formatting invalid day values.

vitaut added inline comments.Jul 16 2022, 8:11 AM
libcxx/include/__chrono/formatter.h
37

module -> modulo

41–42

Submit an LWG issue to clarify this?

52

You could make this more robust and shorter:

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

Is this expensive? Maybe extract a facet lazily since it's often unused.

117–131

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.

libcxx/include/__chrono/ostream.h
36–40

Could you give an example of a format call illustrating the problem on AIX and Windows?

libcxx/include/__chrono/parser_std_format_spec.h
86

a -> an

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

libcxx/include/__chrono/formatter.h
41–42

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.

80

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.

117–131

Agreed. For now I've been mainly looking at getting things to work. But optimizing it most definitely on my todo list.

libcxx/include/__chrono/ostream.h
36–40

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);

Mordante updated this revision to Diff 445344.Jul 17 2022, 12:37 PM

Attempts to fix the CI.

Mordante marked 5 inline comments as done.Jul 17 2022, 10:59 PM
Mordante added inline comments.
libcxx/include/__chrono/formatter.h
61–62

@vitaut suggested something similar. I like this approach better, modulo the trailing comma.

Mordante updated this revision to Diff 445390.Jul 17 2022, 11:00 PM
Mordante marked an inline comment as done.

Addresses review comments and attempts to fix the CI.

Mordante updated this revision to Diff 445529.Jul 18 2022, 8:59 AM

Attempt to fix CI.

ldionne added inline comments.Tue, Jul 26, 9:48 AM
libcxx/include/__chrono/formatter.h
52

I think it might be important to use = tm() to get proper zero initialization, otherwise some members might be left uninitialized.

ldionne requested changes to this revision.Tue, Aug 9, 9:38 AM
ldionne added inline comments.
libcxx/docs/Status/Cxx20Issues.csv
178
libcxx/include/__chrono/formatter.h
51

I don't think this should be in formatter.h or in the __formatter namespace. This seems generally useful, don't you think? I might suggest something like __chrono/convert_to_tm.h to mirror convert_to_timespec.h.

libcxx/include/__chrono/statically_widen.h
14
36

Equivalent since _LIBCPP_STATICALLY_WIDEN should only be used in our namespace, but still more idiomatic since this is a macro.

46

Ditto.

libcxx/include/module.modulemap.in
497

This shouldn't be here.

512

Not needed.

516

Not needed.

libcxx/test/libcxx/utilities/format/format.formatter/format.formatter.spec/formattable.compile.pass.cpp
144

I would prefer instead:

// The chrono formatters require localization support because {XXXXXXXX}
#ifndef TEST_HAS_NO_LOCALIZATION
  assert_is_formattable<std::chrono::day, CharT>();
#endif

We don't need to test that it's not formattable when localization is disabled.

libcxx/test/std/time/time.cal/time.cal.day/time.cal.day.nonmembers/ostream.pass.cpp
35–37

Leftover?

90–91

Leftover?

libcxx/test/std/time/time.syn/formatter_tests.h
8

Missing header guard.

61–67
89–93

I would suggest putting that above so it doesn't clutter the body of the function so much. You could use something like using Context = ..... The same comment applies above too.

104–105
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp
119

Same comment as above for TEST_HAS_NO_LOCALIZATION.

This revision now requires changes to proceed.Tue, Aug 9, 9:38 AM
Mordante updated this revision to Diff 451231.Tue, Aug 9, 12:05 PM
Mordante marked 16 inline comments as done.
  • Rebased
  • Addresses most review comments
  • Qualify several calls
  • Comment out some transitive includes that caused include cycles
Mordante planned changes to this revision.Tue, Aug 9, 12:05 PM
Mordante updated this revision to Diff 451474.Wed, Aug 10, 8:04 AM

Adds work-around to test CI.

Mordante updated this revision to Diff 451552.Wed, Aug 10, 10:46 AM

Attempts to fix Windows build.
Adds convert_to_tm private header.

Mordante updated this revision to Diff 452413.Sat, Aug 13, 5:00 AM

Fixes CI.

Mordante updated this revision to Diff 452491.Sun, Aug 14, 1:05 AM

Attempts to fix CI.

Mordante updated this revision to Diff 452538.Sun, Aug 14, 10:22 AM

Tests failing CI.

Mordante updated this revision to Diff 452696.Mon, Aug 15, 8:55 AM

Attempt to fix the CI.

Mordante updated this revision to Diff 452727.Mon, Aug 15, 10:14 AM

Remove more parts of the experimental library.

Mordante updated this revision to Diff 452756.Mon, Aug 15, 11:41 AM

Another attempt to fix the CI.

Notes from live review:

  1. Let's move transitive includes to the end of each file to break cycles
  2. Let's guard transitive includes on standard versions

Also, please update this to its final form and we can take a look at the Windows DLL errors.

Mordante updated this revision to Diff 453056.Tue, Aug 16, 10:01 AM

Restore CI and try to fix circular dependencies.

Mordante updated this revision to Diff 453074.Tue, Aug 16, 11:02 AM

Disable a test and test CI