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

overflow in constant expression in count_fractional_digits with non-decimal durations #2715

Closed
BrukerJWD opened this issue Jan 13, 2022 · 4 comments

Comments

@BrukerJWD
Copy link
Contributor

BrukerJWD commented Jan 13, 2022

  • fmtlib 8.1.1
  • gcc 10.2

see https://godbolt.org/z/rMvqK5Y99

When trying to format a duration that has non-decimal denominator (e.g. std::chrono::duration<int, std::ratio<3, 7>>), we get compiler errors in gcc due to an overflow when counting the fractional digits:

chrono.h:1669:32:   in 'constexpr' expansion of 'fmt::v8::detail::count_fractional_digits(((long long int)((intmax_t)std::ratio<3, 7>::num)), ((long long int)((intmax_t)std::ratio<3, 7>::den)), 0)'
chrono.h:1479:53:   in 'constexpr' expansion of 'fmt::v8::detail::count_fractional_digits((num * 10), den, (n + 1))'
chrono.h:1479:53:   in 'constexpr' expansion of 'fmt::v8::detail::count_fractional_digits((num * 10), den, (n + 1))'
[...]

It is a quite uncommon use case, but in general fmtlib does support it, e.g. std::chrono::duration<int, std::ratio<3, 10>> is output as [3/10]s.

@vitaut
Copy link
Contributor

vitaut commented Jan 17, 2022

cc @matrackif

@matrackif
Copy link
Contributor

C++11 constexpr functions force us to use recursion instead of loops.

We also cannot have multiple return statements or variables declarations. Therefore we use the ternary operator everywhere, but ternary operator expressions are always evaluated, even when the "false" case is chosen.

The only solution I came up with is to use classic C++98 style recursive template instantiation, where our partial specialization handles the base case.

@vitaut
Copy link
Contributor

vitaut commented Jan 17, 2022

Thanks, @matrackif, for the quick fix!

@vitaut vitaut closed this as completed Jan 17, 2022
@BrukerJWD
Copy link
Contributor Author

Ah, I didn't think about good ol C++11!
Thanks, @matrackif!

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

No branches or pull requests

3 participants