Fix %S formatting for chrono durations with leading zeroes - #3814
Conversation
| EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{99999}), | ||
| "39.99"); |
There was a problem hiding this comment.
If we are rounding than this should probably be 40.00.
There was a problem hiding this comment.
Thinking more of it, we should check what the standard has to say about rounding in this case and, if necessary, open an issue for clarification.
There was a problem hiding this comment.
From what I have read in the standard, there doesn't seem to be anything related to rounding rules for these cases (unless I missed something). There possibly isn't any because allowing a precision modifier with %S for a duration with an integral rep is unique to fmt/not part of the standard?
There was a problem hiding this comment.
The same applies to the floating-point representation and the standard is unclear what to do about it. libc++ currently ignores precision: https://www.godbolt.org/z/7Gxj5hG8v. I asked for clarification, let's see how it goes.
One problem with rounding is that if we round 59.999 seconds we'll get 00 seconds and will have to increment minutes, and so on. I.e. rounding should apply to the argument, not just %S.
So I see two options:
- Truncate
- Round the argument such that all chrono specifiers see the rounded value
In any case we shouldn't round in some cases and truncate in other. I am leaning to the second option since it is consistent with floating-point formatting.
There was a problem hiding this comment.
Sounds good, I have a fix for the rounding ready but I will wait until there is confirmation which way (round/truncate) to go
There was a problem hiding this comment.
In the meantime I suggest going with truncation (round towards zero) since that's the default for duration_cast: https://stackoverflow.com/q/36122428/471164.
There was a problem hiding this comment.
Just wondering, is there a reason why we should follow duration_cast specifically? Because in the case of round (https://en.cppreference.com/w/cpp/chrono/duration/round), it follows the round half to even method. IMO, I feel like rounding makes more sense so it doesn't lose "precision" of the original number.
There was a problem hiding this comment.
The idea was that duration_cast is the closest we can get to some sort of a default but thinking more of it there is nothing special about it compared to other functions. I don't have strong opinion about whether to round or truncate but truncation seems simpler because you don't need to bump minutes etc. Neither method looses precision more than the other.
There was a problem hiding this comment.
Sure that's fair, I pushed the changes to make the rounding into truncation/towards zero now.
| EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{99999}), | ||
| "39.99"); |
There was a problem hiding this comment.
The same applies to the floating-point representation and the standard is unclear what to do about it. libc++ currently ignores precision: https://www.godbolt.org/z/7Gxj5hG8v. I asked for clarification, let's see how it goes.
One problem with rounding is that if we round 59.999 seconds we'll get 00 seconds and will have to increment minutes, and so on. I.e. rounding should apply to the argument, not just %S.
So I see two options:
- Truncate
- Round the argument such that all chrono specifiers see the rounded value
In any case we shouldn't round in some cases and truncate in other. I am leaning to the second option since it is consistent with floating-point formatting.
| EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{99999}), | ||
| "39.99"); |
There was a problem hiding this comment.
In the meantime I suggest going with truncation (round towards zero) since that's the default for duration_cast: https://stackoverflow.com/q/36122428/471164.
|
Thank you! |
2 participants
My attempt to fix #3794. I implemented the rounding as round half to even and fixed the case where an extraneous 0 is added when the decimal part is 0. Added tests as well.
I do have one question: what would be the desired behavior in the case where rounding up the subseconds/decimal part would eventually carry over to the integer part (the seconds) (for example:
01.99rounded to precision .1)? As of right now, all numbers with.9...will not carry over regardless, so from the previous example,01.99 -> 01.99.