Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[libc++][chrono] Implements formatter day.
ClosedPublic

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

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rGe5d2d3eafbb3: [libc++][chrono] Implements formatter day.
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 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.Jul 26 2022, 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.Aug 9 2022, 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.Aug 9 2022, 9:38 AM
Mordante updated this revision to Diff 451231.Aug 9 2022, 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.Aug 9 2022, 12:05 PM
Mordante updated this revision to Diff 451474.Aug 10 2022, 8:04 AM

Adds work-around to test CI.

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

Attempts to fix Windows build.
Adds convert_to_tm private header.

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

Fixes CI.

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

Attempts to fix CI.

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

Tests failing CI.

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

Attempt to fix the CI.

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

Remove more parts of the experimental library.

Mordante updated this revision to Diff 452756.Aug 15 2022, 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.Aug 16 2022, 10:01 AM

Restore CI and try to fix circular dependencies.

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

Disable a test and test CI

Mordante added inline comments.Aug 25 2022, 8:51 AM
libcxx/include/chrono
700

As tested in D132284.

704

The header can be unconditional again.

Mordante updated this revision to Diff 457342.Sep 1 2022, 11:59 AM

Rebased and integrated all upstream changes.

Mordante updated this revision to Diff 457607.Sep 2 2022, 8:31 AM

Attempts to fix CI.

Mordante updated this revision to Diff 457787.Sep 3 2022, 5:58 AM

Rebased and some final polishing.

ldionne accepted this revision.Sep 6 2022, 8:48 AM
ldionne added inline comments.
libcxx/include/chrono
699

Is this comment still required? I think it is no longer needed since we inlined the destructor of format_error to workaround Windows DLL issues.

This revision is now accepted and ready to land.Sep 6 2022, 8:48 AM
Mordante marked 3 inline comments as done.Sep 7 2022, 9:43 AM
Mordante added inline comments.
libcxx/include/chrono
699

No it's not, removed it.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.