From b4f4b7e21a22d1711708fcc90170c654a9e99565 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Sun, 12 Mar 2017 07:30:20 -0700 Subject: [PATCH] Clean the buffer API (#477) --- fmt/format.h | 79 +++++++++++++++++++++++++------------------- fmt/string.h | 13 ++++---- test/ostream-test.cc | 2 +- test/util-test.cc | 14 ++++---- 4 files changed, 59 insertions(+), 49 deletions(-) diff --git a/fmt/format.h b/fmt/format.h index 8ea618c9..502a6ab8 100644 --- a/fmt/format.h +++ b/fmt/format.h @@ -581,18 +581,22 @@ class basic_buffer { private: FMT_DISALLOW_COPY_AND_ASSIGN(basic_buffer); - protected: T *ptr_; std::size_t size_; std::size_t capacity_; - basic_buffer(T *ptr = 0, std::size_t capacity = 0) - : ptr_(ptr), size_(0), capacity_(capacity) {} + protected: + basic_buffer() FMT_NOEXCEPT : ptr_(0), size_(0), capacity_(0) {} + + /** Sets the buffer data and capacity. */ + void set(T* data, std::size_t capacity) FMT_NOEXCEPT { + ptr_ = data; + capacity_ = capacity; + } /** \rst - Increases the buffer capacity to hold at least *capacity* elements updating - ``ptr_`` and ``capacity_``. + Increases the buffer capacity to hold at least *capacity* elements. \endrst */ virtual void grow(std::size_t capacity) = 0; @@ -606,6 +610,9 @@ class basic_buffer { /** Returns the capacity of this buffer. */ std::size_t capacity() const FMT_NOEXCEPT { return capacity_; } + /** Returns a pointer to the buffer data. */ + T *data() FMT_NOEXCEPT { return ptr_; } + /** Returns a pointer to the buffer data. */ const T *data() const FMT_NOEXCEPT { return ptr_; } @@ -693,7 +700,8 @@ class basic_memory_buffer : private Allocator, public basic_buffer { // Deallocate memory allocated by the buffer. void deallocate() { - if (this->ptr_ != store_) Allocator::deallocate(this->ptr_, this->capacity_); + T* data = this->data(); + if (data != store_) Allocator::deallocate(data, this->capacity()); } protected: @@ -701,7 +709,9 @@ class basic_memory_buffer : private Allocator, public basic_buffer { public: explicit basic_memory_buffer(const Allocator &alloc = Allocator()) - : Allocator(alloc), basic_buffer(store_, SIZE) {} + : Allocator(alloc) { + this->set(store_, SIZE); + } ~basic_memory_buffer() { deallocate(); } #if FMT_USE_RVALUE_REFERENCES @@ -710,18 +720,19 @@ class basic_memory_buffer : private Allocator, public basic_buffer { void move(basic_memory_buffer &other) { Allocator &this_alloc = *this, &other_alloc = other; this_alloc = std::move(other_alloc); - this->size_ = other.size_; - this->capacity_ = other.capacity_; - if (other.ptr_ == other.store_) { - this->ptr_ = store_; - std::uninitialized_copy(other.store_, other.store_ + this->size_, - internal::make_ptr(store_, this->capacity_)); + T* data = other.data(); + std::size_t size = other.size(), capacity = other.capacity(); + if (data == other.store_) { + this->set(store_, capacity); + std::uninitialized_copy(other.store_, other.store_ + size, + internal::make_ptr(store_, capacity)); } else { - this->ptr_ = other.ptr_; + this->set(data, capacity); // Set pointer to the inline array so that delete is not called // when deallocating. - other.ptr_ = other.store_; + other.set(other.store_, 0); } + this->resize(size); } public: @@ -754,22 +765,21 @@ class basic_memory_buffer : private Allocator, public basic_buffer { template void basic_memory_buffer::grow(std::size_t size) { - std::size_t new_capacity = this->capacity_ + this->capacity_ / 2; + std::size_t old_capacity = this->capacity(); + std::size_t new_capacity = old_capacity + old_capacity / 2; if (size > new_capacity) new_capacity = size; - T *new_ptr = this->allocate(new_capacity); + T *old_data = this->data(); + T *new_data = this->allocate(new_capacity); // The following code doesn't throw, so the raw pointer above doesn't leak. - std::uninitialized_copy(this->ptr_, this->ptr_ + this->size_, - internal::make_ptr(new_ptr, new_capacity)); - std::size_t old_capacity = this->capacity_; - T *old_ptr = this->ptr_; - this->capacity_ = new_capacity; - this->ptr_ = new_ptr; - // deallocate may throw (at least in principle), but it doesn't matter since - // the buffer already uses the new storage and will deallocate it in case - // of exception. - if (old_ptr != store_) - Allocator::deallocate(old_ptr, old_capacity); + std::uninitialized_copy(old_data, old_data + this->size(), + internal::make_ptr(new_data, new_capacity)); + this->set(new_data, new_capacity); + // deallocate must not throw according to the standard, but even if it does, + // the buffer already uses the new storage and will deallocate it in + // destructor. + if (old_data != store_) + Allocator::deallocate(old_data, old_capacity); } typedef basic_memory_buffer memory_buffer; @@ -793,8 +803,9 @@ class basic_fixed_buffer : public basic_buffer { given size. \endrst */ - basic_fixed_buffer(Char *array, std::size_t size) - : basic_buffer(array, size) {} + basic_fixed_buffer(Char *array, std::size_t size) { + this->set(array, size); + } /** \rst @@ -803,8 +814,9 @@ class basic_fixed_buffer : public basic_buffer { \endrst */ template - explicit basic_fixed_buffer(Char (&array)[SIZE]) - : basic_buffer(array, SIZE) {} + explicit basic_fixed_buffer(Char (&array)[SIZE]) { + this->set(array, SIZE); + } protected: FMT_API void grow(std::size_t size); @@ -2097,8 +2109,7 @@ class basic_context : stored in the object so make sure they have appropriate lifetimes. \endrst */ - basic_context(const Char *format_str, - basic_args args) + basic_context(const Char *format_str, basic_args args) : Base(format_str, args) {} // Parses argument id and returns corresponding argument. diff --git a/fmt/string.h b/fmt/string.h index c0a62510..1f1d8339 100644 --- a/fmt/string.h +++ b/fmt/string.h @@ -48,10 +48,9 @@ class basic_string_buffer : public basic_buffer { std::basic_string str_; protected: - virtual void grow(std::size_t size) { - str_.resize(size); - this->ptr_ = &str_[0]; - this->capacity_ = size; + virtual void grow(std::size_t capacity) { + str_.resize(capacity); + this->set(&str_[0], capacity); } public: @@ -61,10 +60,10 @@ class basic_string_buffer : public basic_buffer { \endrst */ void move_to(std::basic_string &str) { - str_.resize(this->size_); + str_.resize(this->size()); str.swap(str_); - this->capacity_ = this->size_ = 0; - this->ptr_ = 0; + this->resize(0); + this->set(0, 0); } }; diff --git a/test/ostream-test.cc b/test/ostream-test.cc index a3cc4ddb..c0bc4b0d 100644 --- a/test/ostream-test.cc +++ b/test/ostream-test.cc @@ -135,7 +135,7 @@ TEST(OStreamTest, WriteToOStreamMaxSize) { return; struct TestBuffer : fmt::basic_buffer { - explicit TestBuffer(std::size_t size) { size_ = size; } + explicit TestBuffer(std::size_t size) { resize(size); } void grow(std::size_t) {} } buffer(max_size); diff --git a/test/util-test.cc b/test/util-test.cc index 57a74a42..fc805b8f 100644 --- a/test/util-test.cc +++ b/test/util-test.cc @@ -120,21 +120,21 @@ TEST(BufferTest, Nonmoveable) { // A test buffer with a dummy grow method. template struct TestBuffer : basic_buffer { - void grow(std::size_t size) { this->capacity_ = size; } + void grow(std::size_t capacity) { this->set(0, capacity); } }; template struct MockBuffer : basic_buffer { - MOCK_METHOD1(do_grow, void (std::size_t size)); + MOCK_METHOD1(do_grow, void (std::size_t capacity)); - void grow(std::size_t size) { - this->capacity_ = size; - do_grow(size); + void grow(std::size_t capacity) { + this->set(this->data(), capacity); + do_grow(capacity); } MockBuffer() {} - MockBuffer(T *ptr) : basic_buffer(ptr) {} - MockBuffer(T *ptr, std::size_t capacity) : basic_buffer(ptr, capacity) {} + MockBuffer(T *data) { this->set(data, 0); } + MockBuffer(T *data, std::size_t capacity) { this->set(data, capacity); } }; TEST(BufferTest, Ctor) {