-
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
Implement c++20 std::chrono::duration subsecond formatting #2623
Conversation
…g to the c++20 standard
* Allow proper Duration::rep type to propagate via template argument deduction
* Allow proper Duration::rep type to propagate via template argument deduction
@vitaut Hi could you rerun the CI tests. Thanks |
@vitaut I locally used -Wconversion instead of -Wsign-converison so line of code still had to be fixed. If you could run the workflow one more time it would be much appreciated. Thanks |
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.
Thanks for the PR!
test/chrono-test.cc
Outdated
} | ||
using nanoseconds_dbl = std::chrono::duration<double, std::nano>; | ||
EXPECT_EQ(fmt::format("{:%S}", nanoseconds_dbl{-123456789.123456789}), | ||
"-00.123456789"); |
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.
Why is the integral part 00
here?
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.
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")
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.
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.
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.
Done
test/chrono-test.cc
Outdated
@@ -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 |
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.
nit: Please add punctuation (. in the end of the sentence) here and elsewhere.
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.
Done
test/chrono-test.cc
Outdated
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 |
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.
"empty zeros"? I suggest rephrasing this as "zero fractional part" or smth like that.
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.
Done
test/chrono-test.cc
Outdated
"40", | ||
fmt::format("{:%S}", std::chrono::duration<double>(1e20)).substr(0, 2)); |
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.
Why change this?
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.
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")
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.
Shall we check that there is no decimal point then?
include/fmt/chrono.h
Outdated
@@ -1550,11 +1588,11 @@ struct chrono_formatter { | |||
} | |||
} | |||
|
|||
void write(Rep value, int width) { | |||
template <typename RepType> void write(RepType value, int width) { |
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.
Why introduce a template parameter instead of using Rep
?
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.
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.
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.
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).
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.
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
include/fmt/chrono.h
Outdated
uint32_or_64_or_128_t<std::intmax_t> n = | ||
to_unsigned(to_nonnegative_int(value, max_value<std::intmax_t>())); |
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.
Let's use long long
instead of intmax_t
because it's a built-in type and can also handle 18 digits of precision.
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.
As you wish, std::chrono::duration types use std::intmax_t internally that's why I went along with it.
include/fmt/chrono.h
Outdated
@@ -1319,20 +1319,20 @@ inline bool isfinite(T) { | |||
} | |||
|
|||
// Converts value to int and checks that it's in the range [0, upper). |
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.
int -> Int
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.
Done
include/fmt/chrono.h
Outdated
1000); | ||
return std::chrono::duration<Rep, std::milli>(static_cast<Rep>(ms)); | ||
} | ||
template <class Duration> class subsecond_helper { |
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.
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.
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.
Done
Co-authored-by: Victor Zverovich <victor.zverovich@gmail.com>
…ubsecond_helper class with a function
…th name write_fractional_seconds() * Revert write(Rep value, int width) function to previous state
…write_fractional_seconds()
@vitaut I have implemented the suggestions you have requested. If you have any further remarks I would gladly hear them. The workflows still need to be run :) |
test/chrono-test.cc
Outdated
// No decimal point is printed so size() is 2. | ||
EXPECT_EQ(value.size(), 2); | ||
EXPECT_EQ("40", value.substr(0, 2)); |
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.
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.
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.
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
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.
A few more, mostly minor comments.
include/fmt/chrono.h
Outdated
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 |
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.
nit: amount -> 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.
Ok
include/fmt/chrono.h
Outdated
// 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. | ||
static constexpr int num_digits(long long num, long long den, int n = 0) { |
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.
I don't think static is needed here (and similarly in a functions below).
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.
Good point, static made sense when it was a member function, now it is no longer is needed.
inline std::chrono::duration<Rep, std::milli> get_milliseconds( | ||
// 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. |
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.
Is returning 6 actually required by the standard? Can we turn it into a compile-time error instead?
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 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.
include/fmt/chrono.h
Outdated
@@ -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 = |
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.
This doesn't need to be static because it's just an int.
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.
Good point, lightweight integral types probably do not need to be static, even if constexpr
* Remove static from detail functions, they are no longer member functions of a class and static is unnecessary. * Change comment from "amount" to "number"
|
||
template <class Rep, class Period, | ||
FMT_ENABLE_IF(!std::numeric_limits<Rep>::is_signed)> | ||
static constexpr std::chrono::duration<Rep, Period> abs( |
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.
One more static to remove here.
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.
Removed in a follow-up commit.
Merged, thanks! |
A fuzzer has discovered a UB which seems to be introduced by this PR: fmt::print("{:%S}", std::chrono::duration<long double, std::ratio<1, 1000000> >(-1.7591538808701083784E+4865L));
|
Subseconds are always an integral value no longer than 18 digits. The implementation will cast all values to a long long. If the value is not representable by a long long like this huge double UBsan will report this error. It is not clear how to handle values not representable by intmax_t/long long, the spec doesn't seem to mention what to do. I guess it's up to the implementation how to truncate such values. |
I think we should add a range check when converting from an FP number. |
And throw an exception if the value is out of range? What method do you propose for range checking floats. I assume such checks exist already in the codebase |
I implemented a fix for the UB in 784e2a7. |
This commit does not work. I believe it can be safely reverted because currently the above example throws an exception "invalid value". If exceptions are disabled then we need an alternative approach.
Whereas if we change the type to
Also the commit addresses only |
Could you provide a PR with a better workaround for the UB? |
At first I thought of the following solution:
a. If yes, then cast it to its seconds representation, and truncate it. This way we don't subtract away the valuable subsecond part of the number. Something like this:
b. If not, then cast to But then I realized that in practice such code will never be reached because
Will thrown an exception if the value is larger than A possible workaround would be to avoid using Any thoughts? |
This sounds reasonable. We can always optimize this path later and correctness is more important. |
Ok, I am trying to format floating point subsecond values using the floating point formatter, but as always there are issues :). If the duration type is floating point, then we do our calculations using floating point arithmetic. Let's say we calculate the subseconds remaining to be
So we could cast to an integral type to remove the fractional part, but then we end up with the same problem that we started with, since some floating point values are not representable by integral types and we get UB. In other words, how can we remove the fractional part of floating point numbers without casting to an integral type? Or how can we format a FP number without its fractional part (and decimal point), using existing facilities? |
The easy way out would be to simply throw an exception if the formatted value is larger than |
I think we should format integral and fractional parts together. I took a stab at this as part of fixing another UB in d9f045f but improvements are welcome. |
I am a fan of this approach. We simply format the duration using the floating point formatting facility, otherwise we use the integral implementation. |
Hello,
I have returned with a solution to #2207, after dealing with licensing issues in PR #2554. This time I am 100% the author of the code. The solution is simplified, and could be adapted to use if-constexpr if C++17 is detected at compile-time. In fact, the solution I have proposed is working better than the current MSVC implementation which leads to template overflow when
std::atto
is used as a precision. I will open an issue there.std::intmax_t
(the standard type for chrono tick counts) cause program termination withFMT_ASSERT()
. The standard requires us to support up to 18 decimal digits, andstd::int64_t
can store a maximum value of 9,223,372,036,854,775,807 or, 9.2e+18,. Therefore it can represent all 18 digit numbers. However the following assert test had to be removed: