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.

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

Make std::chrono::duration formatting more similar to C++20 standard #2554

Closed
wants to merge 5 commits into from
Closed

Make std::chrono::duration formatting more similar to C++20 standard #2554

wants to merge 5 commits into from

Conversation

matrackif
Copy link
Contributor

Hello,

Currently std::chrono::duration with the %S and %T specifiers is not printed according to the spec. This issue was addressed but later dropped in #2207. Many commits have appeared since then so I have tried to implement a new approach.

Writes the second as a decimal number. If the number of seconds is less than 10, the result is prefixed with 0.
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). The character for the decimal point is localized according to the locale.

Now, the %S specifier only prints with millisecond precision. I have implemented the subsecond extractor, inspired by the MSVC STL implementation of std::format for the hh_mm_ss class. Please see the new unit tests.

Further comments:

  1. The decimal point is not formatted according to the locale
  2. The %T specifier now behaves according to the C++20 standard and should be equivalent to %H:%M:%S
  3. Possible but highly unlikely overflow in the subsecond_helper functions. Not sure if fmt_safe_duration_cast is required and more analysis might be needed.
  4. Floating point subseconds are truncated by casting to std::chrono::duration::period to std::uintmax_t, this behavior is probably acceptable according to the spec as it only requires the value to be as precise as the the std::ratio in std::chrono::duration::period . The MSVC implementation uses std::floor

Comments and suggestions are welcome.

Thanks

Filip Matracki added 4 commits October 19, 2021 19:53
…cision is inevitable in this case?

* Remove constexpr-if statements since the macro FMT_CONSTEXPR depends on c++14 and not on c++17
* Possibly fix bizarre ambiguity on OSX's AppleClang for the detail::count_digits() function according to gabime/spdlog#914
@matrackif
Copy link
Contributor Author

@vitaut Hi do you mind running the workflows one more time for this PR? Thanks

@vitaut
Copy link
Contributor

vitaut commented Oct 30, 2021

Thanks for the PR but I don't think it can be merged in because it incorporates code from other projects and would require license updates.

@vitaut vitaut closed this Oct 30, 2021
@matrackif
Copy link
Contributor Author

@vitaut Thanks for your reply,

I believe Howard Hinnant's MIT license is compatible with the current license. Otherwise, if you would like to see this feature make it in then let me know. We could devise a similar but more original solution in a different PR and resolve issue #2207

Regards

@vitaut
Copy link
Contributor

vitaut commented Oct 31, 2021

The problem is not license compatibility (although {fmt}'s license is substantially different in that it only requires source attribution) but the fact that these are separate licenses so we would need to include both. The two options are:

  1. Writing the suggested improvement from scratch.
  2. Asking the original code copyright owner (Howard?) if they are OK with distributing a portion of their code under the {fmt}'s license.

Since the changes are relatively simple I suggest going with option 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants