-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for subsecond resolution for time_point #2292
Conversation
…e, to compare only up to seconds
@@ -412,7 +412,11 @@ struct formatter<std::chrono::time_point<std::chrono::system_clock, Duration>, | |||
auto format(std::chrono::time_point<std::chrono::system_clock> val, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a template accepting any std::chrono::time_point<std::chrono::system_clock, Period>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or more precisely, the type of the first parameter should be std::chrono::time_point<std::chrono::system_clock, std::chrono::duration<Rep, Period>>
.
include/fmt/chrono.h
Outdated
@@ -412,7 +412,11 @@ struct formatter<std::chrono::time_point<std::chrono::system_clock, Duration>, | |||
auto format(std::chrono::time_point<std::chrono::system_clock> val, | |||
FormatContext& ctx) -> decltype(ctx.out()) { | |||
std::tm time = localtime(val); | |||
return formatter<std::tm, Char>::format(time, ctx); | |||
auto epoch = val.time_since_epoch(); | |||
auto seconds = std::chrono::duration_cast<std::chrono::milliseconds>(epoch).count() / 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just duration_cast<std::chrono::seconds>
include/fmt/chrono.h
Outdated
auto epoch = val.time_since_epoch(); | ||
auto seconds = std::chrono::duration_cast<std::chrono::milliseconds>(epoch).count() / 1000; | ||
auto today = std::chrono::high_resolution_clock::duration(std::chrono::milliseconds(seconds * 1000)); | ||
auto subseconds = std::chrono::duration_cast<std::chrono::nanoseconds>(epoch - today); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do subseconds = epoch - seconds
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Times preceding the epoch (1970-01-01) need to be considered so that subseconds
is never negative.
auto hasT = std::find(tm_format.begin(), tm_format.end(), 'T') != tm_format.end(); | ||
auto writeSubseconds = (hasS || hasT) && (subseconds != 0); | ||
if (writeSubseconds) | ||
tm_format.push_back('.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to check locale an insert the preferred decimal separator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should check, but I don't know how to get the locale in the chrono formatter. I'll try to grep for anything related to locale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locale can be addressed separately, the default is locale-independent anyway.
include/fmt/chrono.h
Outdated
@@ -449,6 +459,11 @@ template <typename Char> struct formatter<std::tm, Char> { | |||
const size_t MIN_GROWTH = 10; | |||
buf.reserve(buf.capacity() + (size > MIN_GROWTH ? size : MIN_GROWTH)); | |||
} | |||
|
|||
if (writeSubseconds) { | |||
buf.append(std::to_string(subseconds)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not add leading zeroes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comments. I did notice I've overlooked some of those, but I was out. Should be fixed now.
It does test for that, and it works. I've tested it like: auto past = std::chrono::system_clock::time_point{-std::chrono::hours{15}};
fmt::print("15 hours before the start of time is: {:%Y:%m:%d %H:%M:%S}\n", past); and I get
|
I think it would have failed if you tried a time with non-zero subseconds. |
See here for a possible implementation of |
You were correct. I've updated the std::tm localtime as well, and set subseconds to be 1s - whatever value they had. Works now for epoch -15h - 15ns as "1969:12:31 09:59:59.999999985". |
Thanks for the links. I've got some friends coming over soon, so I'll take a look tomorrow (or later). Also a way to handle fractional time. |
It has to be separately computed because localtime simply truncates the subseconds and returns whichever second was before that. But it shouldn't be computed twice, that's true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
@@ -412,7 +412,11 @@ struct formatter<std::chrono::time_point<std::chrono::system_clock, Duration>, | |||
auto format(std::chrono::time_point<std::chrono::system_clock> val, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or more precisely, the type of the first parameter should be std::chrono::time_point<std::chrono::system_clock, std::chrono::duration<Rep, Period>>
.
|
||
if (subseconds < subseconds.zero()) { | ||
time = localtime(val - std::chrono::seconds{1}); | ||
subseconds = std::chrono::seconds{1} + subseconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: std::chrono::seconds{1}
-> std::chrono::seconds(1)
in two places for consistency with the rest of the library.
auto width = std::to_string(Period::den).size() - 1; | ||
std::basic_stringstream<Char> os; | ||
os.fill('0'); | ||
os.width(width); | ||
os << subseconds.count(); | ||
auto formatted_ss = os.str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use format
instead of stringstream, something like
auto formatted_ss format("{:0{}}", subseconds.count(), width);
return formatter<std::tm, Char>::format(time, ctx, bytes(formatted_ss));
@@ -449,8 +476,15 @@ template <typename Char> struct formatter<std::tm, Char> { | |||
const size_t MIN_GROWTH = 10; | |||
buf.reserve(buf.capacity() + (size > MIN_GROWTH ? size : MIN_GROWTH)); | |||
} | |||
|
|||
auto buf_end = buf.end() - 1; | |||
if (writeSubseconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writeSubseconds
-> write_subseconds
(naming convention).
@brcha, do you plan to continue working on this PR or shall we close it for now? |
Sorry, I was very busy the last week or so. I'll continue working on this PR ASAP, given that now I have a lot more free time. |
Closing for now but feel free to submit another PR if and whenever you have time. Thanks! |
Or let me know and I'll reopen this one. |
@brcha, @ecorm, if you ever get back to this, I think this hasn't been noticed in the discussion, unless I miss it: the test is one hour off, epoch‒15 hours is 09:00:00, not 10:00:00. At sharp hours, the hour and –(offset) must sum up to 0 mod 24, for both pre- and post-epoch times. For example, epoch‒1 hour is 23:00, ‒(‒1)+23 = 24 ≡ 0 mod 24; epoch+25 hours is 01:00, ‒(+1) + 25 = 24 ≡ 0 mod 24). |
I think making Is it possible to make Or keep |
{fmt} follows the C++ standard (#2207) but you can discard the fractional using |
Fixes #2207
This code:
now outputs: