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.

Sign up for GitHub

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

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

Closed
wants to merge 9 commits into from
Closed

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

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5343eab
fmt::format<Char, T> inefficiency #92
Nov 17, 2015
ad77903
a) Fix format check tests b) Disable friendship for basic_formatbuf
Nov 18, 2015
9148537
Fix gcc errors.
Nov 18, 2015
9ffdec9
Fixed overflow implementation.
Nov 19, 2015
cadb63e
Custom formatbuf implementation. Four commits squashed into one.
Nov 23, 2015
c1ce408
Merge branch 'slow-custom-type-format' of https://github.com/NotImple…
Nov 23, 2015
6625446
Squash last 2 commits.
Nov 24, 2015
69179b9
Merge branch 'slow-custom-type-format' of https://github.com/NotImple…
Nov 24, 2015
7b61173
Remove unnecessary stuff.
Nov 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 53 additions & 3 deletions format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <stdexcept>
#include <string>
#include <map>
#include <ostream>

#ifndef FMT_USE_IOSTREAMS
# define FMT_USE_IOSTREAMS 1
Expand Down Expand Up @@ -2027,6 +2028,11 @@ class BasicWriter {
*/
std::size_t size() const { return buffer_.size(); }

/**
Returns underlying buffer.
*/
Buffer<Char>& buffer() const { return buffer_; }

/**
Returns a pointer to the output buffer content. No terminating null
character is appended.
Expand Down Expand Up @@ -2628,12 +2634,56 @@ class BasicArrayWriter : public BasicWriter<Char> {
typedef BasicArrayWriter<char> ArrayWriter;
typedef BasicArrayWriter<wchar_t> WArrayWriter;

template<class Elem, class Traits = std::char_traits<Elem> >
class basic_formatbuf : public std::basic_streambuf<Elem, Traits> {

typedef typename std::basic_streambuf<Elem, Traits>::int_type int_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

typedef typename std::basic_streambuf<Elem, Traits>::traits_type traits_type;

using std::basic_streambuf<Elem, Traits>::setp;
using std::basic_streambuf<Elem, Traits>::pptr;
using std::basic_streambuf<Elem, Traits>::pbase;
Copy link

Choose a reason for hiding this comment

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

no space?


Buffer<Elem>& buffer_;
Elem* start_;

public:
basic_formatbuf(Buffer<Elem>& buffer) : buffer_(buffer), start_(&buffer[0]) {

setp(start_, start_ + buffer_.capacity());
}

virtual int_type overflow(int_type _Meta = traits_type::eof()) {

if (!traits_type::eq_int_type(_Meta, traits_type::eof())) {

size_t size = pptr() - start_;
buffer_.resize(size);
buffer_.reserve(size * 2);

start_ = &buffer_[0];
start_[size] = traits_type::to_char_type(_Meta);
setp(start_+ size + 1, start_ + size * 2);
}

return _Meta;
}

size_t size() {
return pptr() - start_;
}
};

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

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

BasicStringRef<Char> str(format_buf.size() > 0 ? &buffer[0] : 0, format_buf.size());
internal::Arg arg = internal::MakeValue<Char>(str);
arg.type = static_cast<internal::Arg::Type>(
internal::MakeValue<Char>::type(str));
Expand Down