Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix %S formatting for chrono durations with leading zeroes #3814

Merged
merged 7 commits into from
Feb 7, 2024
Merged

Fix %S formatting for chrono durations with leading zeroes #3814

Show file tree
Hide file tree
Changes from 6 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 Jan 16, 2024
7e96e39
fix type
js324 Jan 16, 2024
e81ef54
add tests and single digit case
js324 Jan 17, 2024
2ce9f9a
cmake change
js324 Jan 17, 2024
2ccc7aa
remove comment
js324 Jan 17, 2024
4d38a66
formatting
js324 Jan 17, 2024
f8b9e79
truncation instead of rounding
js324 Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions include/fmt/chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,15 +1154,48 @@ void write_fractional_seconds(OutputIt& out, Duration d, int precision = -1) {
} else {
*out++ = '.';
leading_zeroes = (std::min)(leading_zeroes, precision);
out = std::fill_n(out, leading_zeroes, '0');
int remaining = precision - leading_zeroes;
if (remaining != 0 && remaining < num_digits) {
n /= to_unsigned(detail::pow10(to_unsigned(num_digits - remaining)));
out = format_decimal<Char>(out, n, remaining).end;
if (remaining < num_digits) {
int num_truncated_digits = num_digits - remaining;
n /= to_unsigned(detail::pow10(to_unsigned(num_truncated_digits) - 1));
const int old_num_digits = detail::count_digits(n);
auto roundingDigit = n % 10;
n /= 10;
if (n) {
if (roundingDigit > 5 || (roundingDigit == 5 && n % 10 % 2 != 0)) {
n += 1;
}
if (old_num_digits == detail::count_digits(n)) {
if (leading_zeroes) {
out = std::fill_n(out, leading_zeroes - 1, '0');
*out++ = '1';
out = std::fill_n(out, remaining, '0');
} else {
n -= 1;
out = format_decimal<Char>(out, n, remaining).end;
}
} else {
out = std::fill_n(out, leading_zeroes, '0');
out = format_decimal<Char>(out, n, remaining).end;
}
} else {
if (roundingDigit >= 5) {
if (leading_zeroes) {
out = std::fill_n(out, leading_zeroes - 1, '0');
*out++ = '1';
out = std::fill_n(out, remaining, '0');
}
} else {
out = std::fill_n(out, leading_zeroes, '0');
}
}
return;
}
out = format_decimal<Char>(out, n, num_digits).end;
remaining -= num_digits;
out = std::fill_n(out, leading_zeroes, '0');
if (n) {
out = format_decimal<Char>(out, n, num_digits).end;
remaining -= num_digits;
}
out = std::fill_n(out, remaining, '0');
}
}
Expand Down
16 changes: 16 additions & 0 deletions test/chrono-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,22 @@ TEST(chrono_test, cpp20_duration_subsecond_support) {
"01.234000");
EXPECT_EQ(fmt::format("{:.6%S}", std::chrono::milliseconds{-1234}),
"-01.234000");
EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{12345}),
"12.34");
EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{12375}),
"12.38");
EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{-12375}),
"-12.38");
EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{12054}),
"12.05");
EXPECT_EQ(fmt::format("{:.1%S}", std::chrono::milliseconds{12054}),
"12.1");
EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{99999}),
"39.99");
Comment on lines +802 to +803
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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:

  • 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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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 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.

Copy link
Contributor

@vitaut vitaut Feb 6, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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.

EXPECT_EQ(fmt::format("{:.2%S}", std::chrono::milliseconds{1000}),
"01.00");
EXPECT_EQ(fmt::format("{:.3%S}", std::chrono::milliseconds{1}),
"00.001");
EXPECT_EQ(fmt::format("{:.3%S}", std::chrono::seconds{1234}), "34.000");
EXPECT_EQ(fmt::format("{:.3%S}", std::chrono::hours{1234}), "00.000");
EXPECT_EQ(fmt::format("{:.5%S}", dms(1.234)), "00.00123");
Expand Down