From 0d145936ece2ccebb98a3a39ae04a5c81d24bc99 Mon Sep 17 00:00:00 2001 From: Murat Toprak <64306017+toprakmurat@users.noreply.github.com> Date: Sat, 9 Aug 2025 18:14:15 +0300 Subject: [PATCH] Handle allocator propagation in basic_memory_buffer::move, Fix #4487 (#4490) * 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. * 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 --- include/fmt/format.h | 41 +++++++++++++++++++++++++++++++++- test/format-test.cc | 52 ++++++++++++++++++++++++++++++++++++++++++- test/mock-allocator.h | 19 +++++++++++++++- 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 25153a4d..bed75195 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); } + + FMT_CONSTEXPR20 friend bool operator==(allocator, allocator) noexcept { + return true; // All instances of this allocator are equivalent. + } + + FMT_CONSTEXPR20 friend bool operator!=(allocator a, allocator b) noexcept { + return !(a == b); + } }; } // namespace detail @@ -826,11 +834,42 @@ class basic_memory_buffer : public detail::buffer { FMT_CONSTEXPR20 ~basic_memory_buffer() { deallocate(); } private: + template + 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 + 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(); + // Perform copy operation, allocators are different + this->resize(size); + detail::copy(data, data + size, this->data()); + return false; + } + return true; + } + // Move data from other to this buffer. FMT_CONSTEXPR20 void move(basic_memory_buffer& other) { - alloc_ = std::move(other.alloc_); T* data = other.data(); size_t size = other.size(), capacity = other.capacity(); + // 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_); diff --git a/test/format-test.cc b/test/format-test.cc index a303cf0d..a835bd16 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,56 @@ 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(); + 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()); + // 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; + buffer2 = std::move(buffer); + 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 32c4caae..868cf6cb 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) {} @@ -72,6 +76,19 @@ template class allocator_ref { return std::allocator_traits::allocate(*alloc_, n); } void deallocate(value_type* p, size_t n) { alloc_->deallocate(p, n); } + + 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_; + } + + FMT_CONSTEXPR20 friend bool operator!=(const allocator_ref& a, + const allocator_ref& b) noexcept { + return !(a == b); + } }; #endif // FMT_MOCK_ALLOCATOR_H_