From ce2f6b858e7b5632752aee595df99c40e971b3d0 Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Tue, 8 Jul 2025 13:44:08 +0300 Subject: [PATCH 1/7] Handle allocator propagation in basic_memory_buffer::move Update `basic_memory_buffer::move` to respect `propagate_on_container_move_assignment`allocator trait. If the allocator should not propagate and differs from the target's allocator, fallback to copying the buffer instead of transferring ownership. This avoids potential allocator mismatch issues and ensures exception safety. --- include/fmt/format.h | 21 ++++++++++++++++++++- test/mock-allocator.h | 11 +++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 1cbd3df4e306..591a28af0775 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -751,6 +751,14 @@ template struct allocator : private std::decay { } void deallocate(T* p, size_t) { std::free(p); } + + friend bool operator==(const allocator&, const allocator&) noexcept { + return true; // All instances of this allocator are equivalent. + } + + friend bool operator!=(const allocator& a, const allocator& b) noexcept { + return !(a == b); + } }; } // namespace detail @@ -828,9 +836,20 @@ class basic_memory_buffer : public detail::buffer { private: // Move data from other to this buffer. FMT_CONSTEXPR20 void move(basic_memory_buffer& other) { - alloc_ = std::move(other.alloc_); + using alloc_traits = std::allocator_traits; T* data = other.data(); size_t size = other.size(), capacity = other.capacity(); + if constexpr (alloc_traits::propagate_on_container_move_assignment::value) { + alloc_ = std::move(other.alloc_); + } else { + if (alloc_ != other.alloc_) { + this->reserve(capacity); + detail::copy(data, data + size, this->data()); + this->resize(size); + return; + } + } + if (data == other.store_) { this->set(store_, capacity); detail::copy(other.store_, other.store_ + size, store_); diff --git a/test/mock-allocator.h b/test/mock-allocator.h index 32c4caae6a34..168d02a6c46a 100644 --- a/test/mock-allocator.h +++ b/test/mock-allocator.h @@ -72,6 +72,17 @@ template class allocator_ref { return std::allocator_traits::allocate(*alloc_, n); } void deallocate(value_type* p, size_t n) { alloc_->deallocate(p, n); } + + friend bool operator==(const allocator_ref& a, const allocator_ref& b) noexcept { + if (a.alloc_ == b.alloc_) return true; + if (a.alloc_ == nullptr || b.alloc_ == nullptr) return false; + + return *a.alloc_ == *b.alloc_; + } + + friend bool operator!=(const allocator_ref& a, const allocator_ref& b) noexcept { + return !(a == b); + } }; #endif // FMT_MOCK_ALLOCATOR_H_ From 49f8b4c19bb446d0e2b67ec22e87cfb630de7edf Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Sat, 12 Jul 2025 00:09:27 +0300 Subject: [PATCH 2/7] Update move ctor to be C++11 compatible and minor fixes --- include/fmt/format.h | 49 +++++++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 591a28af0775..9bf35a31415a 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -752,12 +752,12 @@ template struct allocator : private std::decay { void deallocate(T* p, size_t) { std::free(p); } - friend bool operator==(const allocator&, const allocator&) noexcept { - return true; // All instances of this allocator are equivalent. + friend bool operator==(allocator, allocator) noexcept { + return true; // All instances of this allocator are equivalent. } - friend bool operator!=(const allocator& a, const allocator& b) noexcept { - return !(a == b); + friend bool operator!=(allocator a, allocator b) noexcept { + return !(a == b); } }; @@ -834,22 +834,43 @@ class basic_memory_buffer : public detail::buffer { FMT_CONSTEXPR20 ~basic_memory_buffer() { deallocate(); } private: + template + typename std::enable_if:: + propagate_on_container_move_assignment::value, + bool>::type + allocator_move_impl(basic_memory_buffer& other) { + alloc_ = std::move(other.alloc_); + return true; + } + // If the allocator does not propagate, + // then copy the content from source buffer. + template + typename std::enable_if:: + propagate_on_container_move_assignment::value, + bool>::type + allocator_move_impl(basic_memory_buffer& other) { + T* data = other.data(); + if (alloc_ != other.alloc_ && data != other.store_) { + size_t size = other.size(), capacity = other.capacity(); + // Perform copy operation, allocators are different + this->reserve(size); + detail::copy(data, data + size, this->data()); + this->resize(size); + other.clear(); + return false; + } + return true; + } + // Move data from other to this buffer. FMT_CONSTEXPR20 void move(basic_memory_buffer& other) { using alloc_traits = std::allocator_traits; T* data = other.data(); size_t size = other.size(), capacity = other.capacity(); - if constexpr (alloc_traits::propagate_on_container_move_assignment::value) { - alloc_ = std::move(other.alloc_); - } else { - if (alloc_ != other.alloc_) { - this->reserve(capacity); - detail::copy(data, data + size, this->data()); - this->resize(size); - return; - } + // Replicate the behaviour of std library containers + if (!allocator_move_impl(other)) { + return; } - if (data == other.store_) { this->set(store_, capacity); detail::copy(other.store_, other.store_ + size, store_); From 7e8a9b96792c06a7b69c892588d4581c1ebebc27 Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Sat, 12 Jul 2025 00:21:16 +0300 Subject: [PATCH 3/7] Add test cases for the updated move ctor - Added two test cases `move_ctor_inline_buffer_non_propagating` and `move_ctor_dynamic_buffer_non_propagating` - Added `PropageteOnMove` template parameter to `allocator_ref` class to be compatible with the old test cases - `allocator_ref` now implements `!=` and `==` operators --- test/format-test.cc | 49 ++++++++++++++++++++++++++++++++++++++++++- test/mock-allocator.h | 14 +++++++++---- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/test/format-test.cc b/test/format-test.cc index a303cf0dfa06..17432e2f3e1b 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -307,7 +307,7 @@ TEST(memory_buffer_test, ctor) { EXPECT_EQ(123u, buffer.capacity()); } -using std_allocator = allocator_ref>; +using std_allocator = allocator_ref, true>; TEST(memory_buffer_test, move_ctor_inline_buffer) { auto check_move_buffer = @@ -351,6 +351,53 @@ TEST(memory_buffer_test, move_ctor_dynamic_buffer) { EXPECT_GT(buffer2.capacity(), 4u); } +using std_allocator_n = allocator_ref, false>; + +TEST(memory_buffer_test, move_ctor_inline_buffer_non_propagating) { + auto check_move_buffer = + [](const char* str, + basic_memory_buffer& buffer) { + std::allocator* original_alloc_ptr = buffer.get_allocator().get(); + basic_memory_buffer buffer2( + std::move(buffer)); + EXPECT_EQ(str, std::string(&buffer[0], buffer.size())); + EXPECT_EQ(str, std::string(&buffer2[0], buffer2.size())); + EXPECT_EQ(5u, buffer2.capacity()); + // Allocators should NOT be transferred; they remain distinct instances. + // The original buffer's allocator pointer should still be valid (not + // nullptr). + EXPECT_EQ(original_alloc_ptr, buffer.get_allocator().get()); + EXPECT_NE(original_alloc_ptr, buffer2.get_allocator().get()); + }; + auto alloc = std::allocator(); + basic_memory_buffer buffer( + (std_allocator_n(&alloc))); + const char test[] = "test"; + buffer.append(string_view(test, 4)); + check_move_buffer("test", buffer); + buffer.push_back('a'); + check_move_buffer("testa", buffer); +} + +TEST(memory_buffer_test, move_ctor_dynamic_buffer_non_propagating) { + auto alloc = std::allocator(); + basic_memory_buffer buffer( + (std_allocator_n(&alloc))); + const char test[] = "test"; + buffer.append(test, test + 4); + const char* inline_buffer_ptr = &buffer[0]; + buffer.push_back('a'); + EXPECT_NE(&buffer[0], inline_buffer_ptr); + std::allocator* original_alloc_ptr = buffer.get_allocator().get(); + basic_memory_buffer buffer2(std::move(buffer)); + EXPECT_EQ(buffer.size(), 0); + EXPECT_EQ(std::string(&buffer2[0], buffer2.size()), "testa"); + EXPECT_GT(buffer2.capacity(), 4u); + EXPECT_NE(&buffer2[0], inline_buffer_ptr); + EXPECT_EQ(original_alloc_ptr, buffer.get_allocator().get()); + EXPECT_NE(original_alloc_ptr, buffer2.get_allocator().get()); +} + void check_move_assign_buffer(const char* str, basic_memory_buffer& buffer) { basic_memory_buffer buffer2; diff --git a/test/mock-allocator.h b/test/mock-allocator.h index 168d02a6c46a..bcec3a2c37db 100644 --- a/test/mock-allocator.h +++ b/test/mock-allocator.h @@ -37,7 +37,8 @@ template class mock_allocator { MOCK_METHOD(void, deallocate, (T*, size_t)); }; -template class allocator_ref { +template +class allocator_ref { private: Allocator* alloc_; @@ -48,6 +49,9 @@ template class allocator_ref { public: using value_type = typename Allocator::value_type; + using propagate_on_container_move_assignment = + typename std::conditional::type; explicit allocator_ref(Allocator* alloc = nullptr) : alloc_(alloc) {} @@ -73,14 +77,16 @@ template class allocator_ref { } void deallocate(value_type* p, size_t n) { alloc_->deallocate(p, n); } - friend bool operator==(const allocator_ref& a, const allocator_ref& b) noexcept { + friend bool operator==(const allocator_ref& a, + const allocator_ref& b) noexcept { if (a.alloc_ == b.alloc_) return true; if (a.alloc_ == nullptr || b.alloc_ == nullptr) return false; - return *a.alloc_ == *b.alloc_; + return *a.alloc_ == *b.alloc_; } - friend bool operator!=(const allocator_ref& a, const allocator_ref& b) noexcept { + friend bool operator!=(const allocator_ref& a, + const allocator_ref& b) noexcept { return !(a == b); } }; From fe8d8b6f63eec9491f6ffac6dc998dc95a4c9a08 Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Thu, 31 Jul 2025 23:28:17 +0300 Subject: [PATCH 4/7] fix resizing error in allocator_move_impl --- include/fmt/format.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 9bf35a31415a..16ac40c4097e 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -853,10 +853,8 @@ class basic_memory_buffer : public detail::buffer { if (alloc_ != other.alloc_ && data != other.store_) { size_t size = other.size(), capacity = other.capacity(); // Perform copy operation, allocators are different - this->reserve(size); - detail::copy(data, data + size, this->data()); this->resize(size); - other.clear(); + detail::copy(data, data + size, this->data()); return false; } return true; From 55e1358efa30e6968e57b21e1851236221c7877c Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Fri, 1 Aug 2025 11:56:24 +0300 Subject: [PATCH 5/7] update move ctor tests --- test/format-test.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/format-test.cc b/test/format-test.cc index 17432e2f3e1b..f4b8f9d50c66 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -358,8 +358,11 @@ TEST(memory_buffer_test, move_ctor_inline_buffer_non_propagating) { [](const char* str, basic_memory_buffer& buffer) { std::allocator* original_alloc_ptr = buffer.get_allocator().get(); + const char* original_data_ptr = &buffer[0]; basic_memory_buffer buffer2( std::move(buffer)); + const char* new_data_ptr = &buffer2[0]; + EXPECT_NE(original_data_ptr, new_data_ptr); EXPECT_EQ(str, std::string(&buffer[0], buffer.size())); EXPECT_EQ(str, std::string(&buffer2[0], buffer2.size())); EXPECT_EQ(5u, buffer2.capacity()); @@ -389,7 +392,8 @@ TEST(memory_buffer_test, move_ctor_dynamic_buffer_non_propagating) { buffer.push_back('a'); EXPECT_NE(&buffer[0], inline_buffer_ptr); std::allocator* original_alloc_ptr = buffer.get_allocator().get(); - basic_memory_buffer buffer2(std::move(buffer)); + basic_memory_buffer buffer2; + buffer2 = std::move(buffer); EXPECT_EQ(buffer.size(), 0); EXPECT_EQ(std::string(&buffer2[0], buffer2.size()), "testa"); EXPECT_GT(buffer2.capacity(), 4u); From 4e219ee7c43659f54f7fed4f9f06dd1ab3066051 Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Sun, 3 Aug 2025 11:18:50 +0300 Subject: [PATCH 6/7] remove unused variable and type alias --- include/fmt/format.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 16ac40c4097e..ff59f0dd9396 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -851,7 +851,7 @@ class basic_memory_buffer : public detail::buffer { allocator_move_impl(basic_memory_buffer& other) { T* data = other.data(); if (alloc_ != other.alloc_ && data != other.store_) { - size_t size = other.size(), capacity = other.capacity(); + size_t size = other.size(); // Perform copy operation, allocators are different this->resize(size); detail::copy(data, data + size, this->data()); @@ -862,7 +862,6 @@ class basic_memory_buffer : public detail::buffer { // Move data from other to this buffer. FMT_CONSTEXPR20 void move(basic_memory_buffer& other) { - using alloc_traits = std::allocator_traits; T* data = other.data(); size_t size = other.size(), capacity = other.capacity(); // Replicate the behaviour of std library containers From 5cd0e40cee3e9437e2e5010e9877f57f67103e89 Mon Sep 17 00:00:00 2001 From: Murat Toprak Date: Sat, 9 Aug 2025 10:13:23 +0300 Subject: [PATCH 7/7] add constexpr support for the allocator_move_impl --- include/fmt/format.h | 22 ++++++++++++---------- test/format-test.cc | 1 - test/mock-allocator.h | 8 ++++---- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index ff59f0dd9396..5ebc37499756 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -752,11 +752,11 @@ template struct allocator : private std::decay { void deallocate(T* p, size_t) { std::free(p); } - friend bool operator==(allocator, allocator) noexcept { + FMT_CONSTEXPR20 friend bool operator==(allocator, allocator) noexcept { return true; // All instances of this allocator are equivalent. } - friend bool operator!=(allocator a, allocator b) noexcept { + FMT_CONSTEXPR20 friend bool operator!=(allocator a, allocator b) noexcept { return !(a == b); } }; @@ -835,20 +835,22 @@ class basic_memory_buffer : public detail::buffer { private: template - typename std::enable_if:: - propagate_on_container_move_assignment::value, - bool>::type - allocator_move_impl(basic_memory_buffer& other) { + FMT_CONSTEXPR20 + typename std::enable_if:: + propagate_on_container_move_assignment::value, + bool>::type + allocator_move_impl(basic_memory_buffer& other) { alloc_ = std::move(other.alloc_); return true; } // If the allocator does not propagate, // then copy the content from source buffer. template - typename std::enable_if:: - propagate_on_container_move_assignment::value, - bool>::type - allocator_move_impl(basic_memory_buffer& other) { + FMT_CONSTEXPR20 + typename std::enable_if:: + propagate_on_container_move_assignment::value, + bool>::type + allocator_move_impl(basic_memory_buffer& other) { T* data = other.data(); if (alloc_ != other.alloc_ && data != other.store_) { size_t size = other.size(); diff --git a/test/format-test.cc b/test/format-test.cc index f4b8f9d50c66..a835bd168311 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -394,7 +394,6 @@ TEST(memory_buffer_test, move_ctor_dynamic_buffer_non_propagating) { std::allocator* original_alloc_ptr = buffer.get_allocator().get(); basic_memory_buffer buffer2; buffer2 = std::move(buffer); - EXPECT_EQ(buffer.size(), 0); EXPECT_EQ(std::string(&buffer2[0], buffer2.size()), "testa"); EXPECT_GT(buffer2.capacity(), 4u); EXPECT_NE(&buffer2[0], inline_buffer_ptr); diff --git a/test/mock-allocator.h b/test/mock-allocator.h index bcec3a2c37db..868cf6cb1ea4 100644 --- a/test/mock-allocator.h +++ b/test/mock-allocator.h @@ -77,16 +77,16 @@ class allocator_ref { } void deallocate(value_type* p, size_t n) { alloc_->deallocate(p, n); } - friend bool operator==(const allocator_ref& a, - const allocator_ref& b) noexcept { + FMT_CONSTEXPR20 friend bool operator==(const allocator_ref& a, + const allocator_ref& b) noexcept { if (a.alloc_ == b.alloc_) return true; if (a.alloc_ == nullptr || b.alloc_ == nullptr) return false; return *a.alloc_ == *b.alloc_; } - friend bool operator!=(const allocator_ref& a, - const allocator_ref& b) noexcept { + FMT_CONSTEXPR20 friend bool operator!=(const allocator_ref& a, + const allocator_ref& b) noexcept { return !(a == b); } };