-
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
Fix %S formatting for chrono durations with leading zeroes #3814
Merged
+26
−7
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8227a2f
fix chrono duration rounding with leading zeroes
js324 7e96e39
fix type
js324 e81ef54
add tests and single digit case
js324 2ce9f9a
cmake change
js324 2ccc7aa
remove comment
js324 4d38a66
formatting
js324 f8b9e79
truncation instead of rounding
js324 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we are rounding than this should probably be 40.00.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, is there a reason why we should follow
duration_cast
specifically? Because in the case ofround
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that's fair, I pushed the changes to make the rounding into truncation/towards zero now.