diff --git a/CMakeLists.txt b/CMakeLists.txt index d8dd862c..3e0a6248 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -101,7 +101,7 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "GNU") -Wold-style-cast -Wundef -Wredundant-decls -Wwrite-strings -Wpointer-arith -Wcast-qual -Wformat=2 -Wmissing-include-dirs - -Wcast-align -Wnon-virtual-dtor + -Wcast-align -Wctor-dtor-privacy -Wdisabled-optimization -Winvalid-pch -Woverloaded-virtual -Wconversion -Wswitch-enum diff --git a/include/fmt/compile.h b/include/fmt/compile.h index 2a07d2f6..cb212f20 100644 --- a/include/fmt/compile.h +++ b/include/fmt/compile.h @@ -587,8 +587,7 @@ template format(const CompiledFormat& cf, const Args&... args) { basic_memory_buffer buffer; - detail::buffer& base = buffer; - cf.format(std::back_inserter(base), args...); + cf.format(detail::buffer_appender(buffer), args...); return to_string(buffer); } diff --git a/include/fmt/core.h b/include/fmt/core.h index a8cc2678..3a7b1433 100644 --- a/include/fmt/core.h +++ b/include/fmt/core.h @@ -629,18 +629,18 @@ template class buffer { T* ptr_; size_t size_; size_t capacity_; - bool fixed_; protected: // Don't initialize ptr_ since it is not accessed to save a few cycles. FMT_SUPPRESS_MSC_WARNING(26495) - buffer(size_t sz) FMT_NOEXCEPT : size_(sz), capacity_(sz), fixed_(false) {} + buffer(size_t sz) FMT_NOEXCEPT : size_(sz), capacity_(sz) {} - buffer(T* p = nullptr, size_t sz = 0, size_t cap = 0, - bool fixed = false) FMT_NOEXCEPT : ptr_(p), - size_(sz), - capacity_(cap), - fixed_(fixed) {} + buffer(T* p = nullptr, size_t sz = 0, size_t cap = 0) FMT_NOEXCEPT + : ptr_(p), + size_(sz), + capacity_(cap) {} + + ~buffer() = default; /** Sets the buffer data and capacity. */ void set(T* buf_data, size_t buf_capacity) FMT_NOEXCEPT { @@ -657,7 +657,6 @@ template class buffer { buffer(const buffer&) = delete; void operator=(const buffer&) = delete; - virtual ~buffer() = default; T* begin() FMT_NOEXCEPT { return ptr_; } T* end() FMT_NOEXCEPT { return ptr_ + size_; } @@ -677,24 +676,26 @@ template class buffer { /** Returns a pointer to the buffer data. */ const T* data() const FMT_NOEXCEPT { return ptr_; } - /** - Resizes the buffer. If T is a POD type new elements may not be initialized. - */ - void resize(size_t new_size) { - reserve(new_size); - size_ = new_size; - } - /** Clears this buffer. */ void clear() { size_ = 0; } - /** Reserves space to store at least *capacity* elements. */ - void reserve(size_t new_capacity) { + // Tries resizing the buffer to contain *count* elements. If T is a POD type + // the new elements may not be initialized. + void try_resize(size_t count) { + try_reserve(count); + size_ = count <= capacity_ ? count : capacity_; + } + + // Tries increasing the buffer capacity to *new_capacity*. It can increase the + // capacity by a smaller amount than requested but guarantees there is space + // for at least one additional element either by increasing the capacity or by + // flushing the buffer if it is full. + void try_reserve(size_t new_capacity) { if (new_capacity > capacity_) grow(new_capacity); } void push_back(const T& value) { - reserve(size_ + 1); + try_reserve(size_ + 1); ptr_[size_++] = value; } @@ -707,12 +708,6 @@ template class buffer { } }; -// A fixed capacity buffer. -template class fixed_buffer : buffer { - public: - fixed_buffer(T* data, size_t capacity) : buffer(data, 0, capacity, true) {} -}; - // A container-backed buffer. template class container_buffer : public buffer { @@ -1815,7 +1810,10 @@ OutputIt vformat_to( OutputIt out, const S& format_str, basic_format_args>> args) { auto& c = detail::get_container(out); - detail::container_buffer> buf(c); + using container = remove_reference_t; + typename std::conditional< + std::is_same>::value, + detail::buffer&, detail::container_buffer>::type buf(c); detail::vformat_to(buf, to_string_view(format_str), args); return out; } diff --git a/include/fmt/format-inl.h b/include/fmt/format-inl.h index 39924263..d3970d47 100644 --- a/include/fmt/format-inl.h +++ b/include/fmt/format-inl.h @@ -145,7 +145,7 @@ FMT_FUNC void format_error_code(detail::buffer& out, int error_code, // Report error code making sure that the output fits into // inline_buffer_size to avoid dynamic memory allocation and potential // bad_alloc. - out.resize(0); + out.try_resize(0); static const char SEP[] = ": "; static const char ERROR_STR[] = "error "; // Subtract 2 to account for terminating null characters in SEP and ERROR_STR. @@ -156,7 +156,7 @@ FMT_FUNC void format_error_code(detail::buffer& out, int error_code, ++error_code_size; } error_code_size += detail::to_unsigned(detail::count_digits(abs_value)); - auto it = std::back_inserter(out); + auto it = buffer_appender(out); if (message.size() <= inline_buffer_size - error_code_size) format_to(it, "{}{}", message, SEP); format_to(it, "{}{}", ERROR_STR, error_code); @@ -1051,7 +1051,7 @@ void fallback_format(Double d, buffer& buf, int& exp10) { if (result > 0 || (result == 0 && (digit % 2) != 0)) ++data[num_digits - 1]; } - buf.resize(to_unsigned(num_digits)); + buf.try_resize(to_unsigned(num_digits)); exp10 -= num_digits - 1; return; } @@ -1075,7 +1075,7 @@ int format_float(T value, int precision, float_specs specs, buffer& buf) { buf.push_back('0'); return 0; } - buf.resize(to_unsigned(precision)); + buf.try_resize(to_unsigned(precision)); std::uninitialized_fill_n(buf.data(), precision, '0'); return -precision; } @@ -1113,7 +1113,7 @@ int format_float(T value, int precision, float_specs specs, buffer& buf) { fallback_format(value, buf, exp); return exp; } - buf.resize(to_unsigned(handler.size)); + buf.try_resize(to_unsigned(handler.size)); } else { if (precision > 17) return snprintf_float(value, precision, specs, buf); fp normalized = normalize(fp(value)); @@ -1131,7 +1131,7 @@ int format_float(T value, int precision, float_specs specs, buffer& buf) { ++exp; } } - buf.resize(to_unsigned(num_digits)); + buf.try_resize(to_unsigned(num_digits)); } return exp - cached_exp10; } @@ -1182,19 +1182,20 @@ int snprintf_float(T value, int precision, float_specs specs, ? snprintf_ptr(begin, capacity, format, precision, value) : snprintf_ptr(begin, capacity, format, value); if (result < 0) { - buf.reserve(buf.capacity() + 1); // The buffer will grow exponentially. + // The buffer will grow exponentially. + buf.try_reserve(buf.capacity() + 1); continue; } auto size = to_unsigned(result); // Size equal to capacity means that the last character was truncated. if (size >= capacity) { - buf.reserve(size + offset + 1); // Add 1 for the terminating '\0'. + buf.try_reserve(size + offset + 1); // Add 1 for the terminating '\0'. continue; } auto is_digit = [](char c) { return c >= '0' && c <= '9'; }; if (specs.format == float_format::fixed) { if (precision == 0) { - buf.resize(size); + buf.try_resize(size); return 0; } // Find and remove the decimal point. @@ -1204,11 +1205,11 @@ int snprintf_float(T value, int precision, float_specs specs, } while (is_digit(*p)); int fraction_size = static_cast(end - p - 1); std::memmove(p, p + 1, to_unsigned(fraction_size)); - buf.resize(size - 1); + buf.try_resize(size - 1); return -fraction_size; } if (specs.format == float_format::hex) { - buf.resize(size + offset); + buf.try_resize(size + offset); return 0; } // Find and parse the exponent. @@ -1234,7 +1235,7 @@ int snprintf_float(T value, int precision, float_specs specs, fraction_size = static_cast(fraction_end - begin - 1); std::memmove(begin + 1, begin + 2, to_unsigned(fraction_size)); } - buf.resize(to_unsigned(fraction_size) + offset + 1); + buf.try_resize(to_unsigned(fraction_size) + offset + 1); return exp - fraction_size; } } @@ -1373,7 +1374,8 @@ FMT_FUNC void format_system_error(detail::buffer& out, int error_code, int result = detail::safe_strerror(error_code, system_message, buf.size()); if (result == 0) { - format_to(std::back_inserter(out), "{}: {}", message, system_message); + format_to(detail::buffer_appender(out), "{}: {}", message, + system_message); return; } if (result != ERANGE) diff --git a/include/fmt/format.h b/include/fmt/format.h index e511b7a1..3fd7769d 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -374,7 +374,7 @@ reserve(std::back_insert_iterator it, size_t n) { template inline buffer_appender reserve(buffer_appender it, size_t n) { buffer& buf = get_container(it); - buf.reserve(buf.size() + n); + buf.try_reserve(buf.size() + n); return it; } @@ -572,7 +572,7 @@ template template void buffer::append(const U* begin, const U* end) { size_t new_size = size_ + to_unsigned(end - begin); - reserve(new_size); + try_reserve(new_size); std::uninitialized_copy(begin, end, make_checked(ptr_ + size_, capacity_ - size_)); size_ = new_size; @@ -628,7 +628,7 @@ class basic_memory_buffer : public detail::buffer { } protected: - void grow(size_t size) FMT_OVERRIDE; + void grow(size_t size) final; public: using value_type = T; @@ -638,7 +638,7 @@ class basic_memory_buffer : public detail::buffer { : alloc_(alloc) { this->set(store_, SIZE); } - ~basic_memory_buffer() FMT_OVERRIDE { deallocate(); } + ~basic_memory_buffer() { deallocate(); } private: // Move data from other to this buffer. @@ -682,6 +682,15 @@ class basic_memory_buffer : public detail::buffer { // Returns a copy of the allocator associated with this buffer. Allocator get_allocator() const { return alloc_; } + + /** + Resizes the buffer to contain *count* elements. If T is a POD type new + elements may not be initialized. + */ + void resize(size_t count) { this->try_resize(count); } + + /** Increases the buffer capacity to *new_capacity*. */ + void reserve(size_t new_capacity) { this->try_reserve(new_capacity); } }; template diff --git a/include/fmt/os.h b/include/fmt/os.h index ebe32243..3171dad3 100644 --- a/include/fmt/os.h +++ b/include/fmt/os.h @@ -349,25 +349,26 @@ template void print(direct_buffered_file& f, const S& format_str, const Args&... args); // A buffered file with a direct buffer access and no synchronization. -class direct_buffered_file { +class direct_buffered_file : private detail::buffer { private: file file_; - enum { buffer_size = 4096 }; - char buffer_[buffer_size]; - int pos_; + char buffer_[BUFSIZ]; void flush() { - if (pos_ == 0) return; - file_.write(buffer_, pos_); - pos_ = 0; + if (size() == 0) return; + file_.write(buffer_, size()); + clear(); } - int free_capacity() const { return buffer_size - pos_; } + int free_capacity() const { return static_cast(BUFSIZ - size()); } + + protected: + void grow(size_t) final; public: - direct_buffered_file(cstring_view path, int oflag) - : file_(path, oflag), pos_(0) {} + direct_buffered_file(const char* path, int oflag) + : buffer(buffer_, 0, BUFSIZ), file_(path, oflag) {} ~direct_buffered_file() { flush(); } @@ -379,21 +380,7 @@ class direct_buffered_file { template friend void print(direct_buffered_file& f, const S& format_str, const Args&... args) { - // We could avoid double buffering. - auto buf = fmt::memory_buffer(); - fmt::format_to(std::back_inserter(buf), format_str, args...); - auto remaining_pos = 0; - auto remaining_size = buf.size(); - while (remaining_size > detail::to_unsigned(f.free_capacity())) { - auto size = f.free_capacity(); - memcpy(f.buffer_ + f.pos_, buf.data() + remaining_pos, size); - f.pos_ += size; - f.flush(); - remaining_pos += size; - remaining_size -= size; - } - memcpy(f.buffer_ + f.pos_, buf.data() + remaining_pos, remaining_size); - f.pos_ += static_cast(remaining_size); + fmt::format_to(detail::buffer_appender(f), format_str, args...); } }; #endif // FMT_USE_FCNTL diff --git a/include/fmt/ostream.h b/include/fmt/ostream.h index c16107f7..1b809eb4 100644 --- a/include/fmt/ostream.h +++ b/include/fmt/ostream.h @@ -103,7 +103,7 @@ void format_value(buffer& buf, const T& value, #endif output << value; output.exceptions(std::ios_base::failbit | std::ios_base::badbit); - buf.resize(buf.size()); + buf.try_resize(buf.size()); } // Formats an object of type T that has an overloaded ostream operator<<. diff --git a/include/fmt/printf.h b/include/fmt/printf.h index d4440ed1..8c28ac23 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -181,7 +181,7 @@ template class printf_width_handler { template void vprintf(buffer& buf, basic_string_view format, basic_format_args args) { - Context(std::back_inserter(buf), format, args).format(); + Context(buffer_appender(buf), format, args).format(); } } // namespace detail @@ -598,7 +598,7 @@ OutputIt basic_printf_context::format() { template using basic_printf_context_t = - basic_printf_context>, Char>; + basic_printf_context, Char>; using printf_context = basic_printf_context_t; using wprintf_context = basic_printf_context_t; diff --git a/src/os.cc b/src/os.cc index 386119db..74685549 100644 --- a/src/os.cc +++ b/src/os.cc @@ -124,7 +124,7 @@ void detail::format_windows_error(detail::buffer& out, int error_code, if (result != 0) { utf16_to_utf8 utf8_message; if (utf8_message.convert(system_message) == ERROR_SUCCESS) { - format_to(std::back_inserter(out), "{}: {}", message, utf8_message); + format_to(buffer_appender(out), "{}: {}", message, utf8_message); return; } break; @@ -313,5 +313,9 @@ long getpagesize() { return size; # endif } + +void direct_buffered_file::grow(size_t) { + if (this->size() == BUFSIZ) flush(); +} #endif // FMT_USE_FCNTL FMT_END_NAMESPACE diff --git a/test/core-test.cc b/test/core-test.cc index c53466ff..eb10a8a6 100644 --- a/test/core-test.cc +++ b/test/core-test.cc @@ -34,6 +34,7 @@ using fmt::detail::buffer; using fmt::detail::value; using testing::_; +using testing::Return; using testing::StrictMock; namespace { @@ -84,16 +85,16 @@ template struct test_buffer : buffer { }; template struct mock_buffer : buffer { - MOCK_METHOD1(do_grow, void(size_t capacity)); + MOCK_METHOD1(do_grow, size_t(size_t capacity)); - void grow(size_t capacity) { - this->set(this->data(), capacity); - do_grow(capacity); + void grow(size_t capacity) { this->set(this->data(), do_grow(capacity)); } + + mock_buffer(T* data = nullptr, size_t capacity = 0) { + this->set(data, capacity); + ON_CALL(*this, do_grow(_)) + .WillByDefault( + testing::Invoke([](size_t capacity) { return capacity; })); } - - mock_buffer() {} - mock_buffer(T* data) { this->set(data, 0); } - mock_buffer(T* data, size_t capacity) { this->set(data, capacity); } }; TEST(BufferTest, Ctor) { @@ -120,22 +121,9 @@ TEST(BufferTest, Ctor) { } } -struct dying_buffer : test_buffer { - MOCK_METHOD0(die, void()); - ~dying_buffer() { die(); } - - private: - virtual void avoid_weak_vtable(); -}; - -void dying_buffer::avoid_weak_vtable() {} - -TEST(BufferTest, VirtualDtor) { - typedef StrictMock stict_mock_buffer; - stict_mock_buffer* mock_buffer = new stict_mock_buffer(); - EXPECT_CALL(*mock_buffer, die()); - buffer* buffer = mock_buffer; - delete buffer; +TEST(BufferTest, Indestructible) { + static_assert(!std::is_destructible>(), + "buffer's destructor is protected"); } TEST(BufferTest, Access) { @@ -149,30 +137,39 @@ TEST(BufferTest, Access) { EXPECT_EQ(42, const_buffer[3]); } -TEST(BufferTest, Resize) { +TEST(BufferTest, TryResize) { char data[123]; mock_buffer buffer(data, sizeof(data)); buffer[10] = 42; EXPECT_EQ(42, buffer[10]); - buffer.resize(20); + buffer.try_resize(20); EXPECT_EQ(20u, buffer.size()); EXPECT_EQ(123u, buffer.capacity()); EXPECT_EQ(42, buffer[10]); - buffer.resize(5); + buffer.try_resize(5); EXPECT_EQ(5u, buffer.size()); EXPECT_EQ(123u, buffer.capacity()); EXPECT_EQ(42, buffer[10]); - // Check if resize calls grow. + // Check if try_resize calls grow. EXPECT_CALL(buffer, do_grow(124)); - buffer.resize(124); + buffer.try_resize(124); EXPECT_CALL(buffer, do_grow(200)); - buffer.resize(200); + buffer.try_resize(200); +} + +TEST(BufferTest, TryResizePartial) { + char data[10]; + mock_buffer buffer(data, sizeof(data)); + EXPECT_CALL(buffer, do_grow(20)).WillOnce(Return(15)); + buffer.try_resize(20); + EXPECT_EQ(buffer.capacity(), 15); + EXPECT_EQ(buffer.size(), 15); } TEST(BufferTest, Clear) { test_buffer buffer; - buffer.resize(20); - buffer.resize(0); + buffer.try_resize(20); + buffer.try_resize(0); EXPECT_EQ(static_cast(0), buffer.size()); EXPECT_EQ(20u, buffer.capacity()); } @@ -184,7 +181,7 @@ TEST(BufferTest, Append) { buffer.append(test, test + 5); EXPECT_STREQ(test, &buffer[0]); EXPECT_EQ(5u, buffer.size()); - buffer.resize(10); + buffer.try_resize(10); EXPECT_CALL(buffer, do_grow(12)); buffer.append(test, test + 2); EXPECT_EQ('t', buffer[10]); @@ -196,7 +193,7 @@ TEST(BufferTest, AppendAllocatesEnoughStorage) { char data[19]; mock_buffer buffer(data, 10); const char* test = "abcdefgh"; - buffer.resize(10); + buffer.try_resize(10); EXPECT_CALL(buffer, do_grow(19)); buffer.append(test, test + 9); } @@ -255,7 +252,7 @@ template struct mock_visitor { template struct result { typedef test_result type; }; mock_visitor() { - ON_CALL(*this, visit(_)).WillByDefault(testing::Return(test_result())); + ON_CALL(*this, visit(_)).WillByDefault(Return(test_result())); } MOCK_METHOD1_T(visit, test_result(T value)); diff --git a/test/format-test.cc b/test/format-test.cc index 442ad7a7..31f8595f 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -297,7 +297,7 @@ TEST(MemoryBufferTest, Grow) { mock_allocator alloc; struct TestMemoryBuffer : Base { TestMemoryBuffer(Allocator alloc) : Base(alloc) {} - void grow(size_t size) { Base::grow(size); } + using Base::grow; } buffer((Allocator(&alloc))); buffer.resize(7); using fmt::detail::to_unsigned; diff --git a/test/ostream-test.cc b/test/ostream-test.cc index 3200d46d..bd98f643 100644 --- a/test/ostream-test.cc +++ b/test/ostream-test.cc @@ -151,7 +151,8 @@ TEST(OStreamTest, WriteToOStreamMaxSize) { if (max_size <= fmt::detail::to_unsigned(max_streamsize)) return; struct test_buffer : fmt::detail::buffer { - explicit test_buffer(size_t size) { resize(size); } + explicit test_buffer(size_t size) + : fmt::detail::buffer(nullptr, size, size) {} void grow(size_t) {} } buffer(max_size);