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

Implement c++20 std::chrono::duration subsecond formatting #2623

Merged
merged 14 commits into from
Dec 9, 2021
Merged

Implement c++20 std::chrono::duration subsecond formatting #2623

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c379573
Add support for subsecond printing for std::chrono::duration accordin…
matrackif Nov 28, 2021
57d35cc
Remove assert test that overflows intmax_t
matrackif Nov 28, 2021
863d2ff
* Hopefully fix int64_t to int32_t conversion errors.
matrackif Nov 29, 2021
beacfd3
* Hopefully fix int64_t to int32_t conversion errors.
matrackif Nov 29, 2021
b8e3e33
Fix sign conversion (-Wsign-conversion) warning treated as error in n…
matrackif Dec 4, 2021
5619e87
Format chrono.h with clang-format
matrackif Dec 4, 2021
7544467
Remove extra forward slash in doxygen style comment
matrackif Dec 4, 2021
3d860b7
Apply all suggestions from GitHub, except for replacing the utility s…
matrackif Dec 5, 2021
0fcdb01
* Move logic of handling subseconds from utility class to function wi…
matrackif Dec 5, 2021
55f62cd
Fix -Wshadow warning
matrackif Dec 5, 2021
a0abbed
Remove unsued get_subseconds() function, its logic has been moved to …
matrackif Dec 5, 2021
38fb7a6
Change comment from lowercase int to uppercase Int
matrackif Dec 6, 2021
89c8198
Simplify test check
matrackif Dec 6, 2021
9344c47
Integrate suggested changes
matrackif Dec 9, 2021
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
96 changes: 68 additions & 28 deletions include/fmt/chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1319,20 +1319,20 @@ inline bool isfinite(T) {
}

// Converts value to int and checks that it's in the range [0, upper).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int -> Int

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

template <typename T, FMT_ENABLE_IF(std::is_integral<T>::value)>
inline int to_nonnegative_int(T value, int upper) {
template <typename T, typename Int, FMT_ENABLE_IF(std::is_integral<T>::value)>
vitaut marked this conversation as resolved.
Show resolved Hide resolved
inline Int to_nonnegative_int(T value, Int upper) {
FMT_ASSERT(value >= 0 && to_unsigned(value) <= to_unsigned(upper),
"invalid value");
(void)upper;
return static_cast<int>(value);
return static_cast<Int>(value);
}
template <typename T, FMT_ENABLE_IF(!std::is_integral<T>::value)>
inline int to_nonnegative_int(T value, int upper) {
template <typename T, typename Int, FMT_ENABLE_IF(!std::is_integral<T>::value)>
inline Int to_nonnegative_int(T value, Int upper) {
FMT_ASSERT(
std::isnan(value) || (value >= 0 && value <= static_cast<T>(upper)),
"invalid value");
(void)upper;
return static_cast<int>(value);
return static_cast<Int>(value);
}

template <typename T, FMT_ENABLE_IF(std::is_integral<T>::value)>
Expand Down Expand Up @@ -1389,15 +1389,35 @@ inline std::chrono::duration<Rep, std::milli> get_milliseconds(
#endif
}

template <typename Rep, typename Period,
FMT_ENABLE_IF(std::is_floating_point<Rep>::value)>
inline std::chrono::duration<Rep, std::milli> get_milliseconds(
// Returns the amount of digits according to the c++ 20 spec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: amount -> number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

// In the range [0, 18], if more than 18 fractional digits are required,
// then we return 6 for microseconds precision.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is returning 6 actually required by the standard? Can we turn it into a compile-time error instead?

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 the standard:

...and a precision matching that of the precision of the input (or to a microseconds precision if the conversion to floating-point decimal seconds cannot be made within 18 fractional digits)

Microseconds precision is 6 digits. The MSVC implementation of std::format also behaves this way.

static constexpr int num_digits(long long num, long long den, int n = 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think static is needed here (and similarly in a functions below).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, static made sense when it was a member function, now it is no longer is needed.

return num % den == 0 ? n : (n > 18 ? 6 : num_digits(num * 10, den, n + 1));
}

static constexpr long long pow10(std::uint32_t n) {
return n == 0 ? 1 : 10 * pow10(n - 1);
}

template <class Rep, class Period,
FMT_ENABLE_IF(std::numeric_limits<Rep>::is_signed)>
static constexpr std::chrono::duration<Rep, Period> abs(
std::chrono::duration<Rep, Period> d) {
using common_type = typename std::common_type<Rep, std::intmax_t>::type;
auto ms = mod(d.count() * static_cast<common_type>(Period::num) /
static_cast<common_type>(Period::den) * 1000,
1000);
return std::chrono::duration<Rep, std::milli>(static_cast<Rep>(ms));
// We need to compare the duration using the count() method directly
// due to a compiler bug in clang-11 regarding the spaceship operator,
// when -Wzero-as-null-pointer-constant is enabled.
// In clang-12 the bug has been fixed. See
// https://bugs.llvm.org/show_bug.cgi?id=46235 and the reproducible example:
// https://www.godbolt.org/z/Knbb5joYx
return d.count() >= d.zero().count() ? d : -d;
}

template <class Rep, class Period,
FMT_ENABLE_IF(!std::numeric_limits<Rep>::is_signed)>
static constexpr std::chrono::duration<Rep, Period> abs(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more static to remove here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in a follow-up commit.

std::chrono::duration<Rep, Period> d) {
return d;
}

template <typename Char, typename Rep, typename OutputIt,
Expand Down Expand Up @@ -1560,6 +1580,38 @@ struct chrono_formatter {
out = format_decimal<char_type>(out, n, num_digits).end;
}

template <class Duration> void write_fractional_seconds(Duration d) {
static constexpr auto fractional_width =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be static because it's just an int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, lightweight integral types probably do not need to be static, even if constexpr

detail::num_digits(Duration::period::num, Duration::period::den);

using subsecond_precision = std::chrono::duration<
typename std::common_type<typename Duration::rep,
std::chrono::seconds::rep>::type,
std::ratio<1, detail::pow10(fractional_width)>>;
// We could use c++ 17 if constexpr here.
if (std::ratio_less<typename subsecond_precision::period,
std::chrono::seconds::period>::value) {
*out++ = '.';
const auto subseconds =
std::chrono::treat_as_floating_point<
typename subsecond_precision::rep>::value
? (detail::abs(d) -
std::chrono::duration_cast<std::chrono::seconds>(d))
.count()
: std::chrono::duration_cast<subsecond_precision>(
detail::abs(d) -
std::chrono::duration_cast<std::chrono::seconds>(d))
.count();
uint32_or_64_or_128_t<long long> n =
to_unsigned(to_nonnegative_int(subseconds, max_value<long long>()));
int num_digits = detail::count_digits(n);
if (fractional_width > num_digits) {
out = std::fill_n(out, fractional_width - num_digits, '0');
}
out = format_decimal<char_type>(out, n, num_digits).end;
}
}

void write_nan() { std::copy_n("nan", 3, out); }
void write_pinf() { std::copy_n("inf", 3, out); }
void write_ninf() { std::copy_n("-inf", 4, out); }
Expand Down Expand Up @@ -1637,19 +1689,7 @@ struct chrono_formatter {

if (ns == numeric_system::standard) {
write(second(), 2);
#if FMT_SAFE_DURATION_CAST
// convert rep->Rep
using duration_rep = std::chrono::duration<rep, Period>;
using duration_Rep = std::chrono::duration<Rep, Period>;
auto tmpval = fmt_safe_duration_cast<duration_Rep>(duration_rep{val});
#else
auto tmpval = std::chrono::duration<Rep, Period>(val);
#endif
auto ms = get_milliseconds(tmpval);
if (ms != std::chrono::milliseconds(0)) {
*out++ = '.';
write(ms.count(), 3);
}
write_fractional_seconds(std::chrono::duration<rep, Period>{val});
return;
}
auto time = tm();
Expand Down Expand Up @@ -1678,7 +1718,7 @@ struct chrono_formatter {
on_24_hour_time();
*out++ = ':';
if (handle_nan_inf()) return;
write(second(), 2);
on_second(numeric_system::standard);
}

void on_am_pm() {
Expand Down
49 changes: 44 additions & 5 deletions test/chrono-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,15 +543,14 @@ TEST(chrono_test, negative_durations) {
}

TEST(chrono_test, special_durations) {
EXPECT_EQ(
"40.",
fmt::format("{:%S}", std::chrono::duration<double>(1e20)).substr(0, 3));
auto value = fmt::format("{:%S}", std::chrono::duration<double>(1e20));
// No decimal point is printed so size() is 2.
EXPECT_EQ(value.size(), 2);
EXPECT_EQ("40", value.substr(0, 2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simplify this:

EXPECT_EQ(value, "40");

Then we don't need a comment because it's clear that we don't emit a decimal point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. I recall the size of this string being platform dependent but after the changes in this PR it should always be consistent. Done

auto nan = std::numeric_limits<double>::quiet_NaN();
EXPECT_EQ(
"nan nan nan nan nan:nan nan",
fmt::format("{:%I %H %M %S %R %r}", std::chrono::duration<double>(nan)));
(void)fmt::format("{:%S}",
std::chrono::duration<float, std::atto>(1.79400457e+31f));
EXPECT_EQ(fmt::format("{}", std::chrono::duration<float, std::exa>(1)),
"1Es");
EXPECT_EQ(fmt::format("{}", std::chrono::duration<float, std::atto>(1)),
Expand Down Expand Up @@ -585,4 +584,44 @@ TEST(chrono_test, weekday) {
}
}

TEST(chrono_test, cpp20_duration_subsecond_support) {
using attoseconds = std::chrono::duration<long long, std::atto>;
// Check that 18 digits of subsecond precision are supported.
EXPECT_EQ(fmt::format("{:%S}", attoseconds{999999999999999999}),
"00.999999999999999999");
EXPECT_EQ(fmt::format("{:%S}", attoseconds{673231113420148734}),
vitaut marked this conversation as resolved.
Show resolved Hide resolved
"00.673231113420148734");
EXPECT_EQ(fmt::format("{:%S}", attoseconds{-673231113420148734}),
"-00.673231113420148734");
EXPECT_EQ(fmt::format("{:%S}", std::chrono::nanoseconds{13420148734}),
"13.420148734");
EXPECT_EQ(fmt::format("{:%S}", std::chrono::nanoseconds{-13420148734}),
"-13.420148734");
EXPECT_EQ(fmt::format("{:%S}", std::chrono::milliseconds{1234}), "01.234");
{
// Check that {:%H:%M:%S} is equivalent to {:%T}.
auto dur = std::chrono::milliseconds{3601234};
auto formatted_dur = fmt::format("{:%T}", dur);
EXPECT_EQ(formatted_dur, "01:00:01.234");
EXPECT_EQ(fmt::format("{:%H:%M:%S}", dur), formatted_dur);
}
using nanoseconds_dbl = std::chrono::duration<double, std::nano>;
EXPECT_EQ(fmt::format("{:%S}", nanoseconds_dbl{-123456789}), "-00.123456789");
EXPECT_EQ(fmt::format("{:%S}", nanoseconds_dbl{9123456789}), "09.123456789");
// Verify that only the seconds part is extracted and printed.
EXPECT_EQ(fmt::format("{:%S}", nanoseconds_dbl{99123456789}), "39.123456789");
EXPECT_EQ(fmt::format("{:%S}", nanoseconds_dbl{99123000000}), "39.123000000");
{
// Now the hour is printed, and we also test if negative doubles work.
auto dur = nanoseconds_dbl{-99123456789};
auto formatted_dur = fmt::format("{:%T}", dur);
EXPECT_EQ(formatted_dur, "-00:01:39.123456789");
EXPECT_EQ(fmt::format("{:%H:%M:%S}", dur), formatted_dur);
}
// Check that durations with precision greater than std::chrono::seconds have
// fixed precision, and print zeros even if there is no fractional part.
EXPECT_EQ(fmt::format("{:%S}", std::chrono::microseconds{7000000}),
"07.000000");
}

#endif // FMT_STATIC_THOUSANDS_SEPARATOR