Add color format_to overloads #1843

Naios · 2020-08-31T01:39:08Z

Fix variable size basic_memory_buffer colorization
Rewrite the color unit tests to test the same colors against multiple backends
Ref fmt/color.h: fmt::format_to not implemented for fmt::text_style #1842
Ref fmt/color.h missing format_to overload #1593

Compile time formatted strings are not supported (yet)
The underlying buffer type the colorization is working on was changed to detail::buffer to also accept iterator_buffer, and non default internal size memory buffers. Size limited buffers are not supported (and also not accepted by the public API). This is why format_to_n is not added as part of this change. Additionally since a colorized string is not an atomic unit per character anymore (because it can't be split in the middle of the ansi color code effectively) it doesnt make sence to add support for format_to_n anyway.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

vitaut

Thanks for the PR.

vitaut · 2020-09-01T14:01:28Z

include/fmt/color.h

+// GCC 8 and earlier cannot handle std::back_insert_iterator<Container> with
+// vformat_to<ArgFormatter>(...) overload, so SFINAE on iterator type instead.


Is this a copy-paste? I don't think it is correct.

vitaut · 2020-09-01T14:02:27Z

include/fmt/color.h

+
+   std::vector<char> out;
+   fmt::format_to(std::back_inserter(out),
+                  fmt::emphasis::bold | fg(fmt::color::red), "{}", 42); \endrst


Please put \endrst on a separate line.

vitaut · 2020-09-01T14:02:52Z

include/fmt/color.h

+template <typename OutputIt, typename S, typename Char = char_t<S>,
+          FMT_ENABLE_IF(detail::is_output_iterator<OutputIt>::value)>
+OutputIt vformat_to(
+    OutputIt out, const text_style& ts, const S& format_str,


S should be replaced with basic_string_view

S can't be replaced because basic_string_view is missing an implicit constructor for a fixed size arrays like const char(&)[5]

It doesn't matter. format_to should convert S into basic_string_view when calling this function.

It seems like to_string_view can convert more types than the basic_string_view constructor.
In that case the basic_string_view constructor needs to be aligned with to_string_view or we need to rely on to_string_view to convert it for us. Replacing const S& with basic_string_view will lead to build failures otherwise.

We should use to_string_view.

vitaut · 2020-09-01T14:03:39Z

include/fmt/color.h

+template <typename S, typename... Args, size_t SIZE = inline_buffer_size,
+          typename Char = enable_if_t<detail::is_string<S>::value, char_t<S>>>
+inline typename buffer_context<Char>::iterator format_to(
+    basic_memory_buffer<Char, SIZE>& buf, const text_style& ts,
+    const S& format_str, Args&&... args) {
+  basic_format_args<buffer_context<type_identity_t<Char>>> store =
+      fmt::make_args_checked<Args...>(format_str, args...);
+  detail::vformat_to(buf, ts, to_string_view(format_str), store);
+  return detail::buffer_appender<Char>(buf);
+}


I don't think this is necessary. Please remove.

Thank you for your feedback. I corrected my PR accordingly.
I really would like to keep this overload though, mainly it is the main reason why I submitted this PR.

This overload integrates well into a struct fmt::formatter< specializations for custom types,
it is also possible to colorize only parts of a string and for multiple colorized parts a memory allocation adds up fast.

Otherwise we would have to use the output iterator overload for writing into a memory_buffer although the private color API accepts a memory_buffer already but is inaccessible. This would add unneccessary code overhead (and probably adds a performance overhead too).

This overload is redundant because it's equivalent to passing back_insert_iterator to memory_buffer. There should be no overhead.

vitaut · 2020-09-01T14:07:36Z

test/color-test.cc

+struct ColorFunctionPrintTag {};
+struct ColorFunctionFormatTag {};
+struct ColorFunctionFormatToOutputTag {};
+struct ColorFunctionFormatToNTag {};
+struct ColorFunctionFormatToBufferTag {};
+
+template <typename T> struct ColorsTest;
+template <> struct ColorsTest<ColorFunctionPrintTag> : public testing::Test {
+  virtual ~ColorsTest();
+
+  template <typename... Args>
+  void expect_color(const std::string& expected, const fmt::text_style& ts,
+                    Args&&... args) {
+    EXPECT_WRITE(stdout, fmt::print(stdout, ts, std::forward<Args>(args)...),
+                 expected);
+  }
+};


This is an overkill - please leave the old tests as is and add two test cases that check the new APIs.

Naios · 2020-09-01T16:00:37Z

@vitaut I'm encountering some strange unit test failures on travis (after I've changed back the unit tests to the original style) with some GCC compilers (GCC-6+) that fail to format fmt::format_to(std::back_inserter(out), fg(fmt::terminal_color::red), "{}", "foo"). Do you have an idea about this?

/home/travis/build/fmtlib/fmt/test/color-test.cc:140: Failure
Value of: "\x1b[31mfoo\x1b[0m"
  Actual: "\x1B[31mfoo\x1B[0m"
Expected: format_to_out_helper(fg(fmt::terminal_color::red), "{}", "foo")
Which is: "\x1B[31m{}\x1B[0m"

https://travis-ci.org/github/fmtlib/fmt/jobs/723128246#L747-L761

I'm using a helper funtion as following:

template <typename S, typename... Args,
          typename String = std::basic_string<fmt::char_t<S>>>
String format_to_out_helper(const fmt::text_style& ts, const S& format_str,
                            Args&&... args) {
  String out;
  fmt::format_to(std::back_inserter(out), ts, format_str,
                 std::forward<Args>(args)...);
  return out;
}

// ....

EXPECT_EQ(format_to_out_helper(fg(fmt::terminal_color::red), "{}", "foo"),
          "\x1b[31mfoo\x1b[0m");

The other functions for colorization seem fine.

vitaut

Mostly looks good but please use basic_string_view in format_to. Also there is no need to copy all the tests because all methods share the same implementation. It's enough to do basic sanity test that format_to and vformat_to accept text styles. Then helper functions can go away.

* Fix variable size basic_memory_buffer colorization * Fix an unused arguments warning on GCC that blocks the CI otherwise * Ref fmtlib#1842 * Ref fmtlib#1593

Naios · 2020-09-04T22:46:32Z

I reduced the test cases as requested and now it only does basic tests whether the API accepts fmt::text_style through the format_to function.
Also I could fix the format failures across some compiler with additional format args.

If I understand you right about basic_string_view you mean that I shall replace

template <typename S, typename... Args, size_t SIZE = inline_buffer_size,
          typename Char = enable_if_t<detail::is_string<S>::value, char_t<S>>>
inline typename buffer_context<Char>::iterator format_to(
    basic_memory_buffer<Char, SIZE>& buf, const text_style& ts,
    const S& format_str, Args&&... args) {
  detail::vformat_to(buf, ts, to_string_view(format_str),
                     fmt::make_args_checked<Args...>(format_str, args...));
  return detail::buffer_appender<Char>(buf);
}

by

template <typename Char, typename... Args, size_t SIZE = inline_buffer_size>
inline typename buffer_context<Char>::iterator format_to(
    basic_memory_buffer<Char, SIZE>& buf, const text_style& ts,
    basic_string_view<Char> format_str, Args&&... args) {
  detail::vformat_to(buf, ts, format_str,
                     fmt::make_args_checked<Args...>(format_str, args...));
  return detail::buffer_appender<Char>(buf);
}

which is, from my point of view, not possible because as mentioned above the basic_string_view constructor is not capable to accept all string like types (char const (&)[21]) as to_string_view is. Thus overload resolution will fail for this case and prefers the non text_style overload instead.
Originally I just copied both format_to overloads from format.h and adapted it to accept text_style. I would prefer if this PR could be merged and then afterwards the format_str type can be changed in all format_to overloads.

I attached you my compiler output which could further explain my arguments:

In file included from /home/src/test/color-test.cc:8:
In file included from /home/src/include/fmt/color.h:11:
/home/src/include/fmt/format.h:3562:23: error: no matching function for call to 'make_args_checked'
  const auto& vargs = fmt::make_args_checked<Args...>(format_str, args...);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/src/test/color-test.cc:107:8: note: in instantiation of function template specialization 'fmt::v7::format_to<fmt::v7::text_style, char const (&)[21], int, int, int, 500, char>' requested here
  fmt::format_to(out, ts, "rgb(255,20,30){}{}{}", 1, 2, 3);
       ^
/home/src/include/fmt/core.h:1631:13: note: candidate template ignored: substitution failure [with Args = <char const (&)[21], int, int, int>, S = fmt::v7::text_style]: no type named 'type' in 'fmt::v7::detail::char_t_impl<fmt::v7::text_style, void>'
inline auto make_args_checked(const S& format_str,
            ^
In file included from /home/src/test/color-test.cc:8:
In file included from /home/src/include/fmt/color.h:11:
/home/src/include/fmt/format.h:3563:34: error: no matching function for call to 'to_string_view'
  return detail::vformat_to(buf, to_string_view(format_str), vargs);
                                 ^~~~~~~~~~~~~~
/home/src/include/fmt/core.h:459:32: note: candidate template ignored: could not match 'const Char *' against 'fmt::v7::text_style'
inline basic_string_view<Char> to_string_view(const Char* s) {
                               ^
/home/src/include/fmt/core.h:464:32: note: candidate template ignored: could not match 'basic_string<type-parameter-0-0, type-parameter-0-1, type-parameter-0-2>' against 'const fmt::v7::text_style'
inline basic_string_view<Char> to_string_view(
                               ^
/home/src/include/fmt/core.h:470:32: note: candidate template ignored: could not match 'basic_string_view<type-parameter-0-0>' against 'fmt::v7::text_style'
inline basic_string_view<Char> to_string_view(basic_string_view<Char> s) {
                               ^
/home/src/include/fmt/core.h:476:32: note: candidate template ignored: could not match 'std_string_view<type-parameter-0-0>' against 'fmt::v7::text_style'
inline basic_string_view<Char> to_string_view(detail::std_string_view<Char> s) {
                               ^
/home/src/include/fmt/core.h:488:52: note: candidate template ignored: requirement 'is_compile_string<fmt::v7::text_style>::value' was not satisfied [with S = fmt::v7::text_style]
constexpr basic_string_view<typename S::char_type> to_string_view(const S& s) {
                                                   ^
2 errors generated.
[6/8] Linking CXX static library libfmtd.a
ninja: build stopped: subcommand failed.

vitaut · 2020-09-06T15:30:05Z

I just copied both format_to overloads from format.h and adapted it to accept text_style.

Unfortunately those are not good examples and need to change too eventually. We shouldn't repeat the same mistakes in new code.

vitaut · 2020-09-06T15:44:19Z

Merged in bff4d18 with minor tweaks. Thanks again.

Naios · 2020-09-06T16:20:07Z

Thank you for merging this PR partially.

The back_insert_iterator overload is sadly by far not equivalent to the suggested fmt::memory_buffer overload which you have dropped. Passing a back_insert_iterator always will insert content character-wise whereas using a fmt::memory_buffer overload will use append and passes the data in chunks.

The transfer to iterator_buffer might be buffered, but its flush to the back-inserted container is not, and always happens based on the value_type and will happen character-wise in case of strings (or fmt::memory_buffer).

Calling the API for colorization is now also unintentional because the non fmt::text_style overload accepts a fmt::memory_buffer whereas the fmt::text_style overload now requires a std::back_inserter(...)

Additionally colorization can't be used easily anymore in struct fmt::formatter< without this overload.

vitaut · 2020-09-06T16:32:57Z

The back_insert_iterator overload is sadly by far not equivalent to the suggested fmt::memory_buffer overload which you have dropped.

It can easily be made equivalent if it's not already. The main point is that extra overload is redundant and shouldn't be there.

the non fmt::text_style overload accepts a fmt::memory_buffer whereas the fmt::text_style overload now requires a std::back_inserter(...)

That's fine. The other overload is there mostly for historical reasons.

Additionally colorization can't be used easily anymore in struct fmt::formatter< without this overload.

This is not the case because formatter specialization use iterators.

Naios · 2020-09-06T16:54:44Z

Ok, I see your point now.

Overall the overload is not equal for std::back_inserter because the insertion will happen per character.
But it is possible to supply a custom output iterator which could insert the data chunk-wise.

vitaut · 2020-09-06T17:05:40Z

the insertion will happen per character.

This is only because there is a missing get_buffer[_init] overload that should map back_insert_iterator to the underlying buffer.

Naios · 2020-09-06T17:47:06Z

I have to revert my previous statement. iterator_buffer::flush is currently implemented as following:

template <typename OutputIt, typename T, typename Traits>
void iterator_buffer<OutputIt, T, Traits>::flush() {
  out_ = std::copy_n(data_, this->limit(this->size()), out_);
  this->clear();
}

The (buffered) data is handed to the output one by one theoretically.
Maybe the standard library can do an optimization for common standard library containers wrapped inside a std::back_insert_iterator like std::vector but it is likely that it will fall back to fmt::memory_buffer::push_back when encountering std::back_insert_iterator<fmt::memory_buffer>.

Without it, every output iterator accepting API will never be equal to the same API using a fmt::memory_buffer directly.

Mapping a std::back_insert_iterator<T> to T is probably not possible due to a missing container getter in the standard, and everything else will be implementation dependent.

What could fix this issue is to add support for an output iterator accepting a range of char.

vitaut · 2020-09-06T17:54:15Z

Mapping a std::back_insert_iterator to T is probably not possible due to a missing container getter in the standard, and everything else will be implementation dependent.

It is possible and already implemented in {fmt} with a minor caveat mentioned earlier.

Naios force-pushed the master branch 6 times, most recently from 2a04a32 to 2b66f74 Compare August 31, 2020 12:50

vitaut requested changes Sep 1, 2020

View reviewed changes

Naios force-pushed the master branch 5 times, most recently from 840b114 to c0f3fb3 Compare September 1, 2020 15:53

vitaut requested changes Sep 2, 2020

View reviewed changes

Naios force-pushed the master branch from c0f3fb3 to 96486fe Compare September 4, 2020 21:59

Add color format_to overloads

9d44ba4

* Fix variable size basic_memory_buffer colorization * Fix an unused arguments warning on GCC that blocks the CI otherwise * Ref fmtlib#1842 * Ref fmtlib#1593

Naios force-pushed the master branch from 96486fe to 9d44ba4 Compare September 4, 2020 22:31

vitaut closed this Sep 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add color format_to overloads #1843

Add color format_to overloads #1843

Naios commented Aug 31, 2020 •

edited

Loading

vitaut left a comment

vitaut Sep 1, 2020

vitaut Sep 1, 2020

vitaut Sep 1, 2020

Naios Sep 1, 2020 •

edited

Loading

vitaut Sep 1, 2020

Naios Sep 1, 2020

vitaut Sep 1, 2020

vitaut Sep 1, 2020 •

edited

Loading

Naios Sep 1, 2020

vitaut Sep 6, 2020

vitaut Sep 1, 2020

Naios commented Sep 1, 2020 •

edited

Loading

vitaut left a comment

Naios commented Sep 4, 2020 •

edited

Loading

vitaut commented Sep 6, 2020

vitaut commented Sep 6, 2020

Naios commented Sep 6, 2020 •

edited

Loading

vitaut commented Sep 6, 2020

Naios commented Sep 6, 2020 •

edited

Loading

vitaut commented Sep 6, 2020

Naios commented Sep 6, 2020 •

edited

Loading

vitaut commented Sep 6, 2020

		// GCC 8 and earlier cannot handle std::back_insert_iterator<Container> with
		// vformat_to<ArgFormatter>(...) overload, so SFINAE on iterator type instead.

Add color format_to overloads #1843

Add color format_to overloads #1843

Conversation

Naios commented Aug 31, 2020 • edited Loading

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios commented Sep 1, 2020 • edited Loading

vitaut left a comment

Choose a reason for hiding this comment

Naios commented Sep 4, 2020 • edited Loading

vitaut commented Sep 6, 2020

vitaut commented Sep 6, 2020

Naios commented Sep 6, 2020 • edited Loading

vitaut commented Sep 6, 2020

Naios commented Sep 6, 2020 • edited Loading

vitaut commented Sep 6, 2020

Naios commented Sep 6, 2020 • edited Loading

vitaut commented Sep 6, 2020

Naios commented Aug 31, 2020 •

edited

Loading

Naios Sep 1, 2020 •

edited

Loading

vitaut Sep 1, 2020 •

edited

Loading

Naios commented Sep 1, 2020 •

edited

Loading

Naios commented Sep 4, 2020 •

edited

Loading

Naios commented Sep 6, 2020 •

edited

Loading

Naios commented Sep 6, 2020 •

edited

Loading

Naios commented Sep 6, 2020 •

edited

Loading