Page MenuHomePhabricator

[libc++][chrono] Implements formatter duration.
AcceptedPublic

Authored by Mordante on Sep 27 2022, 8:10 AM.

Details

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

Partially implements:

  • P1361 Integration of chrono with text formatting
  • P2372 Fixing locale handling in chrono formatters
  • LWG3270 Parsing and formatting %j with durations

Completes:

  • P1650R0 std::chrono::days with 'd' suffix
  • LWG3262 Formatting of negative durations is not specified
  • LWG3314 Is stream insertion behavior locale dependent when Period::type is micro?

Diff Detail

Event Timeline

Mordante created this revision.Sep 27 2022, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 8:10 AM
Mordante updated this revision to Diff 463250.Sep 27 2022, 8:57 AM

CI fixes.

Mordante updated this revision to Diff 466373.Oct 9 2022, 7:03 AM

Attempts to fix CI.

Mordante updated this revision to Diff 466374.Oct 9 2022, 7:09 AM

Fix dependencies.

Mordante updated this revision to Diff 468007.Oct 15 2022, 4:01 AM

CI fixes.

Mordante updated this revision to Diff 468009.Oct 15 2022, 4:55 AM

CI fixes.

Mordante updated this revision to Diff 468013.Oct 15 2022, 5:59 AM

CI fixes.

Mordante updated this revision to Diff 468019.Oct 15 2022, 8:17 AM

CI fixes.

Mordante updated this revision to Diff 468023.Oct 15 2022, 8:44 AM

CI fixes.

Mordante updated this revision to Diff 468061.Oct 16 2022, 4:07 AM

Rebased enable all CI builders and minor polishing.

Mordante updated this revision to Diff 468065.Oct 16 2022, 5:57 AM

CI fixes.

Mordante updated this revision to Diff 468066.Oct 16 2022, 6:07 AM

Fixes typo.

Mordante published this revision for review.Oct 16 2022, 7:49 AM
Mordante added reviewers: ldionne, vitaut.
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 7:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Oct 18 2022, 8:59 AM

LGTM with comments!

libcxx/include/__chrono/convert_to_tm.h
29–32

Suggestion. _ChronoEntity is kind of hand-wavy, but _ChronoCalendarTimePoint is now wrong with this new patch, so let's change it.

libcxx/test/std/time/time.syn/formatter.duration.pass.cpp
554

Also elsewhere.

665
This revision is now accepted and ready to land.Oct 18 2022, 8:59 AM
Mordante marked 3 inline comments as done.Oct 18 2022, 10:03 AM
Mordante updated this revision to Diff 468609.Oct 18 2022, 10:08 AM

Rebased and addresses review comments.

This revision was automatically updated to reflect the committed changes.
h-vetinari added inline comments.
libcxx/docs/Status/Cxx20Papers.csv
130

Extraneous >>>"",<<<

Mordante marked an inline comment as done.Oct 19 2022, 10:28 AM

@h-vetinari Thanks for spotting the issue, I've pushed a fix.

EricWF reopened this revision.Oct 30 2022, 4:31 PM
EricWF added a subscriber: EricWF.

Please fix the undefined behavior mentioned above.

libcxx/include/__chrono/convert_to_tm.h
43

Does this not overflow for a lot of things?

Like hours(duration_cast<hours>(seconds::max()) + hours(1))?

Instead of reverting to raw arithmetic on ints, use chrono to get the values you want.

See https://godbolt.org/z/GYc7Tzn9W

This revision is now accepted and ready to land.Oct 30 2022, 4:31 PM
Mordante added inline comments.Nov 28 2022, 10:41 AM
libcxx/include/__chrono/convert_to_tm.h
43

The value is intended to be time since midnight, but it indeed may overflow.

Unfortunately it's not as simple as you suggest. Casting to hours fails for other small units, for example atto seconds. D138826 addresses the issue.