This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Fixes times before epoch.
ClosedPublic

Authored by Mordante on Jul 10 2023, 10:24 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGb9f3d241f46c: [libc++][format] Fixes times before epoch.
Summary

The number of days should be rounded down, for both positive and
negative times since epoch. The original code truncated, which is
correct for positive values, but wrong for negative values.

Depends on D138826

Diff Detail

Event Timeline

Mordante created this revision.Jul 10 2023, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 10:24 AM
Mordante requested review of this revision.Jul 10 2023, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 10:24 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 540666.Jul 15 2023, 3:03 AM

Rebased to test CI.

Mordante updated this revision to Diff 540673.Jul 15 2023, 4:25 AM

Retrigger CI.

35bit -> 32bit?

Mordante added inline comments.Jul 17 2023, 8:51 AM
libcxx/test/std/time/time.syn/formatter.file_time.pass.cpp
53

@h-vetinari you mean here? This should be 35. I probably should add a link to http://eel.is/c++draft/time.syn.

// convenience typedefs
using nanoseconds  = duration<signed integer type of at least 64 bits, nano>;
using microseconds = duration<signed integer type of at least 55 bits, micro>;
using milliseconds = duration<signed integer type of at least 45 bits, milli>;
using seconds      = duration<signed integer type of at least 35 bits>;
using minutes      = duration<signed integer type of at least 29 bits, ratio<  60>>;
using hours        = duration<signed integer type of at least 23 bits, ratio<3600>>;
h-vetinari added inline comments.Jul 18 2023, 12:05 AM
libcxx/test/std/time/time.syn/formatter.file_time.pass.cpp
53

My mistake, sorry. I thought 32 vs. 35 might be a numpad-off-by-one-row error. "35 bit minimum" sounded like "smallest/oldest representable", but from the standard you cited it, it's not the minimum as in representable time, but the minimum as in "we need at least 35 bits"...

Sorry for the noise.

Mordante added inline comments.Jul 18 2023, 8:31 AM
libcxx/test/std/time/time.syn/formatter.file_time.pass.cpp
53

No problem, I appreciate your feedback!

ldionne accepted this revision.Jul 18 2023, 10:22 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/test/std/time/time.syn/formatter.file_time.pass.cpp
53

I agree, let's add a link to the wording.

This revision is now accepted and ready to land.Jul 18 2023, 10:22 AM
Mordante marked 2 inline comments as done.Jul 18 2023, 12:17 PM
This revision was automatically updated to reflect the committed changes.