fmt::format<Char, T> inefficiency #92

[NotImplemented]

Should we write some tests for this item? What is a proper way to run tests locally?

[mwinterb]

While I haven't looked in depth at all the errors that are occurring, based on some of the error messages, it seems likely that it's because of the removal of this line. It seems like fixing this may require using a temporary internal::MemoryBuffer that's used by basic_formatbuf, and then format a BasicStringRef that's extracted from the MemoryBuffer. Since MemoryBuffer can avoid memory allocations for short strings, it seems like this would meet part of the goals for this issue, even if there would still be some unnecessary copies in the "empty format string" case.

[mwinterb]

To be a bit more helpful on my suggestion, keeping the majority of your change the same, except for changing basic_format's constructor to accept a Buffer directly:

// Formats a value.
template <typename Char, typename T>
void format(BasicFormatter<Char> &f, const Char *&format_str, const T &value) {
  internal::MemoryBuffer<Char, internal::INLINE_BUFFER_SIZE> buffer;

  basic_formatbuf<Char> format_buf(buffer);
  std::basic_ostream<Char> output(&format_buf);
  output << value;
  format_buf.flush();

  BasicStringRef<Char> str(buffer.size() > 0 ? &buffer[0] : NULL, buffer.size());
  internal::Arg arg = internal::MakeValue<Char>(str);
  arg.type = static_cast<internal::Arg::Type>(
        internal::MakeValue<Char>::type(str));
  format_str = f.format(format_str, arg);
}

This passes all of the tests, based on an an older commit, though.

[NotImplemented]

Yes, that will fix. Failing tests were connected with format check.

[vitaut]

@NotImplemented Thanks for the PR, Mike! Just two minor comments in addition to what @mwinterb wrote. The format function should be covered by existing tests, but it might be useful to test basic_formatbuf. You can run the tests locally via the test target (RUN_TESTS in MSVC), e.g.

> make test

[vitaut]

@NotImplemented LGTM and happy to merge. But could you squash the commits first to get rid of the ones that were failing on CI?

[danilaml]

no space?

[vitaut]

Merged, thanks!