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 6 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
94 changes: 64 additions & 30 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,16 +1389,54 @@ 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(
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));
}
template <class Duration> class subsecond_helper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a utility struct seems unnecessary. I suggest converting it into a function (e.g. write_fractional_seconds) or merging the logic into write where it is used.

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

/// Returns the amount of digits according to the c++ 20 spec
/// In the range [0, 18], if more than 18 fractional digits are required,
/// then we return 6 for microseconds precision
matrackif marked this conversation as resolved.
Show resolved Hide resolved
static constexpr int num_digits(std::intmax_t num, std::intmax_t den,
int n = 0) {
return num % den == 0 ? n : (n > 18 ? 6 : num_digits(num * 10, den, n + 1));
}

static constexpr std::intmax_t 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) {
return d >= d.zero() ? d : -d;
}

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) {
return d;
}

public:
static constexpr auto fractional_width =
num_digits(Duration::period::num, Duration::period::den);

using precision = std::chrono::duration<
typename std::common_type<typename Duration::rep,
std::chrono::seconds::rep>::type,
std::ratio<1, pow10(fractional_width)>>;

template <class Rep, class Period>
static constexpr typename precision::rep get_subseconds(
std::chrono::duration<Rep, Period> d) {
return std::chrono::treat_as_floating_point<typename precision::rep>::value
? (abs(d) - std::chrono::duration_cast<std::chrono::seconds>(d))
.count()
: std::chrono::duration_cast<precision>(
abs(d) -
std::chrono::duration_cast<std::chrono::seconds>(d))
.count();
}
};

template <typename Char, typename Rep, typename OutputIt,
FMT_ENABLE_IF(std::is_integral<Rep>::value)>
Expand Down Expand Up @@ -1550,11 +1588,11 @@ struct chrono_formatter {
}
}

void write(Rep value, int width) {
template <typename RepType> void write(RepType value, int width) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce a template parameter instead of using Rep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rep is currently assumed to be an unsigned type. I wanted the proper type to propagate through the code and be deduced in the template. Otherwise there could be an unsigned to signed conversion here which would lead to a -Wconversion error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rep is currently assumed to be an unsigned type.

I don't think this is correct. Rep is the representation type of a duration and it can be signed (in fact it's normally signed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, lowercase rep is used to store the internal duration value, and rep is unsigned to avoid overflow. At first I wanted to use the existing logic in the write() function to format a duration value of type lowercase rep. This is irrelevant now as I reverted this function to its previous state

write_sign();
if (isnan(value)) return write_nan();
uint32_or_64_or_128_t<int> n =
to_unsigned(to_nonnegative_int(value, max_value<int>()));
uint32_or_64_or_128_t<std::intmax_t> n =
to_unsigned(to_nonnegative_int(value, max_value<std::intmax_t>()));
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 use long long instead of intmax_t because it's a built-in type and can also handle 18 digits of precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you wish, std::chrono::duration types use std::intmax_t internally that's why I went along with it.

int num_digits = detail::count_digits(n);
if (width > num_digits) out = std::fill_n(out, width - num_digits, '0');
out = format_decimal<char_type>(out, n, num_digits).end;
Expand Down Expand Up @@ -1637,18 +1675,14 @@ 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)) {
using subsec_helper = detail::subsecond_helper<duration_rep>;
// Could use c++ 17 if constexpr
if (std::ratio_less<typename subsec_helper::precision::period,
std::chrono::seconds::period>::value) {
*out++ = '.';
write(ms.count(), 3);
write(subsec_helper::get_subseconds(duration_rep{val}),
subsec_helper::fractional_width);
}
return;
}
Expand Down Expand Up @@ -1678,7 +1712,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
48 changes: 44 additions & 4 deletions test/chrono-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,14 +544,12 @@ TEST(chrono_test, negative_durations) {

TEST(chrono_test, special_durations) {
EXPECT_EQ(
"40.",
fmt::format("{:%S}", std::chrono::duration<double>(1e20)).substr(0, 3));
"40",
fmt::format("{:%S}", std::chrono::duration<double>(1e20)).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.

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the spec, the decimal point should be printed only if the duration "period" type is more precise than seconds:

If the precision of the input cannot be exactly represented with seconds, then the format is a decimal floating-point number with a fixed format and a precision matching that of the precision of the input

This can be determined statically at compile time and does not depend on runtime values of the duration. The test I modified has seconds precision, so I removed the decimal point.

In my local instance of Visual Studio 2022, the following test passes:

std::format("{:%S}", std::chrono::duration<double>{1.234}) == std::string("01")

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we check that there is no decimal point then?

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 +583,46 @@ TEST(chrono_test, weekday) {
}
}

TEST(chrono_test, cpp20_duration_subsecond_support) {
using attoseconds = std::chrono::duration<std::intmax_t, std::atto>;
// Check that 18 digits of subsecond precision are supported
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please add punctuation (. in the end of the sentence) here and elsewhere.

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

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.123456789}),
"-00.123456789");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the integral part 00 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the spec:

If the number of seconds is less than 10, the result is prefixed with 0

The number of seconds is 0, which is less than 10, hence we write 00.

The MSVC implementation of std::format also interprets the standard this way. The check below passes locally in my instance of Visual Studio 2022

std::format("{:%S}", std::chrono::duration<double, std::nano{-123456789.123456789}) == std::string("-00.123456789")

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I misread the test case and thought that the integral part 123456789. is seconds rather than nanoseconds. I suggest changing the fractional part to something different to prevent confusion.

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

EXPECT_EQ(fmt::format("{:%S}", nanoseconds_dbl{9123456789.123456789}),
"09.123456789");
// Only seconds part is 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 empty zeros
Copy link
Contributor

Choose a reason for hiding this comment

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

"empty zeros"? I suggest rephrasing this as "zero fractional part" or smth like that.

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

EXPECT_EQ(fmt::format("{:%S}", std::chrono::microseconds{7000000}),
"07.000000");
}

#endif // FMT_STATIC_THOUSANDS_SEPARATOR