This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Mordante on Sep 18 2022, 8:33 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG1522f190eb2c: [libc++][chrono] Implements formatter month.
Summary

Partially implements:

  • P1361 Integration of chrono with text formatting

Diff Detail

Event Timeline

Mordante created this revision.Sep 18 2022, 8:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2022, 8:33 AM
Mordante updated this revision to Diff 461081.Sep 18 2022, 9:52 AM

CI fixes.

Mordante updated this revision to Diff 461213.Sep 19 2022, 8:05 AM

Fixes CI.

Mordante updated this revision to Diff 462921.Sep 26 2022, 8:37 AM

Rebased and enabled full CI.

Mordante updated this revision to Diff 462928.Sep 26 2022, 8:51 AM

Fixes modular build.

Mordante published this revision for review.Sep 27 2022, 8:09 AM
Mordante added reviewers: ldionne, vitaut.
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 8:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Oct 4 2022, 9:39 AM
ldionne added inline comments.
libcxx/include/__chrono/formatter.h
179

For watchers, this looks quite weird right now but Mark explained that this was going to be extended for other values, and the logic will become non-trivial. So I think this is mandated indeed.

211
libcxx/include/__chrono/ostream.h
51

Can you somehow indicate that this is likely a bug in the spec, not in our implementation? As currently worded, the comment seems to imply that we're doing something wrong.

libcxx/test/std/time/time.cal/time.cal.month/time.cal.month.nonmembers/ostream.pass.cpp
11

Here and elsewhere.

17–18

Can you double-check that these tests are getting run at all in our CI? It seems rather suspicious to me that the no-localization CI job didn't fail despite using the incorrect // UNSUPPORTED: libcpp-has-no-localization formulation above.

20–21
This revision is now accepted and ready to land.Oct 4 2022, 9:39 AM
Mordante updated this revision to Diff 465402.Oct 5 2022, 8:02 AM

Rebased and addresses review comments.

Mordante updated this revision to Diff 465404.Oct 5 2022, 8:07 AM
Mordante edited the summary of this revision. (Show Details)

Rebased.

This revision was automatically updated to reflect the committed changes.
Mordante marked 4 inline comments as done.
Mordante marked an inline comment as done.Oct 5 2022, 9:46 AM
Mordante added inline comments.
libcxx/test/std/time/time.cal/time.cal.month/time.cal.month.nonmembers/ostream.pass.cpp
17–18

I verified and these tests are executed in the CI.
(I would be surprised if they weren't since a lot of the platform specific output have been found by the CI.)