fmt icon indicating copy to clipboard operation
fmt copied to clipboard

Printed seconds precision does not match C++20 spec

Open ecorm opened this issue 4 years ago • 5 comments

According to https://eel.is/c++draft/time.format#tab:time.format.spec, under the %S specifier:

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 (or to a microseconds precision if the conversion to floating-point decimal seconds cannot be made within 18 fractional digits).

When outputting a std::chrono::system_clock::time_point (with resolution in nanoseconds) via the %S format specifier, fmt currently only prints the whole seconds, and does not add the decimal fraction according to the C++20 spec.

Godbolt demo: https://godbolt.org/z/h4rfo5zTr

It outputs 01, where it should be 01.234000000 according to the spec.

Using hinnant/date, I get the expected result:

    using time_type = typename std::chrono::system_clock::time_point;
    time_type t{std::chrono::milliseconds{1234}};
    auto s = date::format(std::locale::classic(), "%S", t);
    fmt::print("{}\n", s); // prints 01.234000000

If one wants to print whole seconds with a system_clock::time_point, they first have to floor it to a sys_time<std::chrono::seconds>.

This problem also affects the %T specifier.

ecorm avatar Apr 01 '21 22:04 ecorm

Thanks for reporting. This is a known limitation of the current implementation relying on conversion to tm which has a seconds resolution.

The solution would be to parse format specs ourselves and handle %S specially. PRs are welcome!

vitaut avatar Apr 02 '21 17:04 vitaut

I've just wrote & tested this blob down, and it works in C++11 (and above). I could find where something similar could be plugged into fmt. Probably only the subsecond part is needed, since the rest has already be handled by tm.

https://godbolt.org/z/rPv3oK6WE

brcha avatar May 18 '21 10:05 brcha

Here's the PR: https://github.com/fmtlib/fmt/pull/2292

brcha avatar May 18 '21 12:05 brcha

@brcha The precision is supposed to match that of the duration passed in. For milliseconds, it should be 3 decimal places. As per the C++ spec:

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 (or to a microseconds precision if the conversion to floating-point decimal seconds cannot be made within 18 fractional digits).

It also says this about giving a precision specifier:

Giving a precision specification in the chrono-format-spec is valid only for std​::​chrono​::​duration types where the representation type Rep is a floating-point type. For all other Rep types, an exception of type format_­error is thrown if the chrono-format-spec contains a precision specification.

Hinnant does compile-time log10 calculations in his library to determine the precision when given the period (i.e. ratio) of a duration with integral rep.

ecorm avatar May 18 '21 16:05 ecorm

Thanks to @matrackif subsecond formatting now works for durations (#2623). We still need to do the same for time points so keeping the issue open.

vitaut avatar Dec 09 '21 15:12 vitaut

Added a pull request to format time points: https://github.com/fmtlib/fmt/pull/3115

patrickroocks avatar Sep 26 '22 13:09 patrickroocks

My pull request https://github.com/fmtlib/fmt/pull/3115 was merged now, so subseconds are now formatted for time points.

patrickroocks avatar Oct 13 '22 06:10 patrickroocks

Thanks, @patrickroocks.

vitaut avatar Oct 13 '22 23:10 vitaut

@vitaut, would you consider releasing a new version that includes this important fix?

I'm embarrassed to say how many hours I spent last week failing to output subsecond-precision timestamps. I settled on printing the %T part of my timestamp as a separate tp.time_since_epoch() formatting parameter.

eli-b avatar Nov 14 '22 23:11 eli-b

I don't think this warrants making a new release but you can pin to a specific commit where this feature is implemented.

vitaut avatar Nov 15 '22 03:11 vitaut

I will, thanks.

For the sake of people who search the issues for something like "printing the milliseconds / microseconds part of a chrono time_point doesn't work", I wrote the text in the quotes so this issue would come up in the search.

eli-b avatar Nov 15 '22 16:11 eli-b

Seconding the wish for a new release. We use this lib via vcpkg so pinning to a specific commit is a hassle. Hoping for a small new release :)

vebjornjr avatar May 08 '23 13:05 vebjornjr