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

Improve compile-time formatting #4118

Closed

Conversation

AnthonyVH
Copy link
Contributor

This PR improves compile-time formatting. More specifically:

  • A few FMT_CONSTEXPR(20) specifiers were added.
  • detail_exported::fixed_string type is now formattable and has a UDL operator.
  • The compile-time fmt::format functions now generate the same error as the runtime ones when a type is not formattable.
  • An extra check was added to one of the write() overload's enable_ifs to avoid ambiguity when a type is unformattable during compile-time formatting.
  • The compile-time formatting tests are now generated as type-parametrized tests, for both formatting to a fixed length buffer, as well as to an std::string_view. This requires some rather ugly macros, but they allow avoiding duplicating all those tests.

@AnthonyVH AnthonyVH force-pushed the fix_compile_time_formatting branch 3 times, most recently from 9eb429f to d83c56f Compare August 14, 2024 07:18
test/util.h Outdated
Comment on lines 85 to 103
// FOR_EACH macro that applies a given function to each variadic macro argument.
// FOREACH macro helpers
#define FOR_EACH_0(WHAT)
#define FOR_EACH_1(WHAT, X) WHAT(X)
#define FOR_EACH_2(WHAT, X, ...) WHAT(X), FOR_EACH_1(WHAT, __VA_ARGS__)
#define FOR_EACH_3(WHAT, X, ...) WHAT(X), FOR_EACH_2(WHAT, __VA_ARGS__)
#define FOR_EACH_4(WHAT, X, ...) WHAT(X), FOR_EACH_3(WHAT, __VA_ARGS__)
#define FOR_EACH_5(WHAT, X, ...) WHAT(X), FOR_EACH_4(WHAT, __VA_ARGS__)
#define FOR_EACH_6(WHAT, X, ...) WHAT(X), FOR_EACH_5(WHAT, __VA_ARGS__)
#define FOR_EACH_7(WHAT, X, ...) WHAT(X), FOR_EACH_6(WHAT, __VA_ARGS__)
#define FOR_EACH_8(WHAT, X, ...) WHAT(X), FOR_EACH_7(WHAT, __VA_ARGS__)
#define FOR_EACH_9(WHAT, X, ...) WHAT(X), FOR_EACH_8(WHAT, __VA_ARGS__)

#define FOR_EACH_GET_MACRO(_1, _2, _3, _4, _5, _6, _7, _8, _9, NAME, ...) NAME
#define FOR_EACH(action, ...) \
FOR_EACH_GET_MACRO(__VA_ARGS__ __VA_OPT__(, ) FOR_EACH_9, FOR_EACH_8, \
FOR_EACH_7, FOR_EACH_6, FOR_EACH_5, FOR_EACH_4, \
FOR_EACH_3, FOR_EACH_2, FOR_EACH_1, FOR_EACH_0) \
(action __VA_OPT__(, ) __VA_ARGS__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not introduce this and avoid new macros in tests.

Copy link
Contributor Author

@AnthonyVH AnthonyVH Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case getting rid of the macros that means duplicating all the tests when testing for a format function that returns an std::string_view. I'm not sure which of two evils is better.

AFAIK, a constexpr function returning the result of a format as std::string_view necessarily has to take all arguments as NTTP, since it needs to store the formatted result as a static constexpr, i.e. one such constant per unique set of arguments. And the issue there is that we can't pass "bla" as NTTP, it needs to be some kind of fixed_string.

I can't figure out a way to do this conditionally per variadic NTTP. As soon as the compiler sees a const char[] it's game over. Hence why I did the conversion of const char[] by wrapping each template argument in a lambda call with this FOR_EACH macro.

Would you prefer I duplicate some/all test cases?

Even better of course would be a way to make the compiler conert the const char[] NTTP to fixed_string automagically.

Copy link
Contributor Author

@AnthonyVH AnthonyVH Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind the above, I figured out a way to get rid of the FOR_EACH and do the conditional NTTP conversion from a char array to a fixed_string without macros.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't think we need those string_view / NTTP tests since they are testing the same logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree, except I'm seeing different compile errors with the NTTP tests vs the existing ones (mainly missing constexpr specifiers).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide an example on godbolt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure: https://godbolt.org/z/eqorr8qWr. Note that there's a bunch of things this example doesn't support. It's just a MWE of one of the differences I saw.

Of course this could be due to something I implemented wrongly. E.g. formatting 42 uses a different call chain than uint64_t{42}, and I might have been going down a bunch of paths trying to make everything work with NTTP. Might have confused myself at some point 😄.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the example. Looks like the only thing that is missing from the coverage is formatted_size (https://godbolt.org/z/8411Eaoh3) so let's revert all the test changes here and add a call to it to test_format, something like:

  constexpr auto n = fmt::formatted_size(format, args...);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also added a missing test case to catch the missing constexpr in the FMT_FORMAT_AS macro.

#endif

#if defined(__cpp_constexpr) && (__cpp_constexpr >= 202211L) && \
defined(FMT_CONSTEXPR20) && FMT_USE_NONTYPE_TEMPLATE_ARGS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FMT_CONSTEXPR20 is always defined:

fmt/include/fmt/base.h

Lines 133 to 139 in fb07b37

#if FMT_USE_CONSTEVAL
# define FMT_CONSTEVAL consteval
# define FMT_CONSTEXPR20 constexpr
#else
# define FMT_CONSTEVAL
# define FMT_CONSTEXPR20
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove it, the other checks guarantee C++23 anyway.

@AnthonyVH AnthonyVH force-pushed the fix_compile_time_formatting branch 4 times, most recently from 3bb35ec to a0de34d Compare August 18, 2024 19:58
@AnthonyVH
Copy link
Contributor Author

AnthonyVH commented Aug 19, 2024

By the way, do you feel there's any value in having a compile-time format to std::string_view available in the API? I.e.:

template <fmt::detail_exported::fixed_string format, nttp_arg... args>
constexpr std::string_view format();

I feel it's not entirely trivial to come up with this (due to not being able to pass char[] const as NTTP), so it might benefit users to have this available as a ready-made function for them.

The implementation would be more or less what I had in this Godbolt example: https://godbolt.org/z/eqorr8qWr.

namespace fmt {
  namespace detail {
    template <typename T> struct nttp_arg {
      template <typename U = T> constexpr nttp_arg(U const& arg) : arg(arg) {}
      T arg;
    };
    
    template <typename T>
    nttp_arg(const T&) -> nttp_arg<T>;
    
    template <typename Char, size_t N>
    nttp_arg(const Char (&arg)[N]) -> nttp_arg<fmt::detail_exported::fixed_string<Char, N>>;
  } // namespace detail

  // Can't be consteval due to llvm bug: https://github.com/llvm/llvm-project/issues/82994.
  template <fmt::detail_exported::fixed_string format, fmt::detail::nttp_arg... args>
  constexpr std::string_view format() {
    static constexpr auto str = [] consteval {
      using namespace fmt::literals;
      constexpr auto compiled_format = operator""_cf<format>();
      constexpr auto result_length =
          fmt::formatted_size(compiled_format, (args.arg)...);
      auto result = std::array<char, result_length>{};
      fmt::format_to(result.data(), compiled_format, (args.arg)...);
      return result;
    }();
    return std::string_view(str.data(), str.size());
  }
} // namespace fmt

@vitaut
Copy link
Contributor

vitaut commented Aug 19, 2024

This diff is really hard to review because it keeps changing and contains unrelated changes. Please split it into individual self-contained PRs. And I don't think we need anything to do with NTTP in the API.

@vitaut vitaut closed this Aug 19, 2024
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.

4 participants