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

formatted_size() is broken for modern compile-time API #2141

Closed
alexezeder opened this issue Feb 21, 2021 · 1 comment · Fixed by #2161
Closed

formatted_size() is broken for modern compile-time API #2141

alexezeder opened this issue Feb 21, 2021 · 1 comment · Fixed by #2161

Comments

@alexezeder
Copy link
Contributor

There is a problem using formatted_size() with format strings for compile-time API - FMT_COMPILE and _cf. Here Is an example - https://godbolt.org/z/zz3c1E, and a simplified version of it - https://godbolt.org/z/drW1rG.

This problem can be solved in at least two ways:

  1. By adding SFINAE check to

    fmt/include/fmt/compile.h

    Lines 837 to 840 in ab0f7d7

    template <typename CompiledFormat, typename... Args>
    size_t formatted_size(const CompiledFormat& cf, const Args&... args) {
    return format_to(detail::counting_iterator(), cf, args...).count();
    }
    so it would not be available for compiled_string-based format strings. Thus the version from core.h will be chosen.
  2. By adding a new formatted_size() implementation specifically for compiled_string-based format strings. This also requires marking a conversion operator in compiled_string-based format strings explicit, which results in changing few lines in compile.h for conversion (with using "custom" FMT_STRING_IMPL macro).

There is no difference between these 2 ways in performance because of compilers optimizations. But there is a difference between runtime API and compile-time API (lack of support for named spec'ed fields, for example). So which way should we choose to fix broken formatted_size()?

Maybe it's not even a problem because, maybe, a format string from compile-time API shouldn't be used in formatted_size().

@vitaut
Copy link
Contributor

vitaut commented Feb 24, 2021

I think we should go with the second option except that instead of making the conversion operator explicit the formatted_size overload should be templatized like other formatting functions.

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 a pull request may close this issue.

2 participants