Skip to content

Make ranges constexpr formattable - #4979

Closed
ilazaric wants to merge 2 commits into
fmtlib:mainfrom
ilazaric:ilazaric/rg
Closed

ilazaric wants to merge 2 commits into
fmtlib:mainfrom
ilazaric:ilazaric/rg

Conversation

@ilazaric

@ilazaric ilazaric commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Added ranges-formatting FMT_COMPILE tests were not working previously. This is because of a discrepancy between compile.h and format.h, former was using std::back_inserter(std::string), while latter was using back_appender(basic_memory_buffer).

This is actually needed because a different part has a bug:

  template <typename FormatContext, typename F, typename... T>
  FMT_CONSTEXPR auto write(FormatContext& ctx, const F& f, T&&... values) const
      -> decltype(ctx.out()) {
    auto specs = specs_;
    handle_dynamic_spec(specs.dynamic_width(), specs.width, width_ref_, ctx);
    if (specs.width == 0) return f.write_body(ctx, static_cast<T&&>(values)...);

    auto buf = basic_memory_buffer<Char>();
    auto buffer_ctx =
        FormatContext(basic_appender<Char>(buf), ctx.args(), ctx.locale());
    f.write_body(buffer_ctx, static_cast<T&&>(values)...);
    return detail::write<Char>(
        ctx.out(), basic_string_view<Char>(buf.data(), buf.size()), specs);
  }

The snippet tries to construct a FormatContext using a different iterator type.
It didn't seem obvious how to fix it, as ctx.args() is also context specific.

This PR is a cleanup of #4956

Added ranges-formatting FMT_COMPILE tests were not working previously.
This is because of a discrepancy between compile.h and format.h,
former was using `std::back_inserter(std::string)`,
while latter was using `back_appender(basic_memory_buffer)`.
@ilazaric
ilazaric requested a review from vitaut as a code owner October 9, 2026 03:23
Comment thread include/fmt/compile.h
Comment on lines +482 to +484
auto buf = basic_memory_buffer<Char>();
cf.format(basic_appender<Char>(buf), args...);
return std::basic_string<Char>(buf.data(), buf.size());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may regress perf for common cases by introducing a copy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would argue there is no perf regression as the current state does not compile, so notion of performance falls apart. But I agree it is not optimal, such a fix seems slightly more involved though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The regression affects existing use cases that compile and work today, not just the newly supported range formatting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just rebased locally, killed my compile.h change, and tested compile_test. Builds and passes. Seems the issue has been fixed already, so closing this PR as unnecessary.

@ilazaric ilazaric closed this Oct 24, 2026
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