Speed up iterator_buffer

[user202729]

Just a 1-line change to reuse the fmt::detail::copy() method instead of manually loop over each character. For that, some methods need to be moved around.

Demonstration of the speedup in fmtlib/format-benchmark#35 . Arguably vector<char> is a rare type, but it might be useful for other things (custom string types...?)

To make tests pass, I need to check whether the iterator type is move-assignable (throwing_iterator is not assignable), although I'd argue that in practice iterators ought to be move-assignable and that test is actually invalid.

[vitaut]

Thanks for the PR.

[vitaut]

Let's move the definition to fmt/format.h instead of bringing all the copy machinery here.

[vitaut]

@user202729 do you still plan to update this PR?

[user202729]

Updated.

That said, it leads to a bunch of other things such as format_to being moved to format.h, which is technically a breaking change since, for example, previously a program that only use base.h would be able to use iterator_buffer, now they can't. (See the failing tests changed in the latest commit. In particular, deleting the error base-test includes format.h is extraordinarily wrong. One option is to move them to format-test.cc.)

I can think of a few possibilities such as providing a faster overload in format.h, but that would probably be a ODR violation.

What do you think?

Side note, if it makes you feel better, you could just close the PR and comment like "if you want to work on this later you can reopen it", or make a [stale] label and set your pull requests page to by default filter them out. I appreciate that you are very active and keep the number of pending issues and pull requests extraordinarily low.

[user202729]

I followed the suggestion, and sure enough, base-test can't link.

What then? Move the tests in base-test.cc that requires this method to format-test?

[vitaut]

I see, the problems is that the definition of flush won't get instantiated with the output iterator type. I guess moving copy to base.h is indeed the simplest option, sorry for going back and forth on it.

[user202729]

in retrospect, with #4716 , this wouldn't be too useful. (maybe std::deque, but for that one the append method is named append_range.) but anyway...?