Handle allocator propagation in basic_memory_buffer::move, Fix fmtlib#4487 (fmtlib#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

Author: toprakmurat

include/fmt/format.h

@@ -751,6 +751,14 @@ template <typename T> struct allocator : private std::decay<void> {
   }
 
   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<T> {
   FMT_CONSTEXPR20 ~basic_memory_buffer() { deallocate(); }
 
  private:
+  template <typename Alloc = Allocator>
+  FMT_CONSTEXPR20
+      typename std::enable_if<std::allocator_traits<Alloc>::
+                                  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 Alloc = Allocator>
+  FMT_CONSTEXPR20
+      typename std::enable_if<!std::allocator_traits<Alloc>::
+                                  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<T>(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<T>(other.store_, other.store_ + size, store_);

test/format-test.cc

@@ -307,7 +307,7 @@ TEST(memory_buffer_test, ctor) {
   EXPECT_EQ(123u, buffer.capacity());
 }
 
-using std_allocator = allocator_ref<std::allocator<char>>;
+using std_allocator = allocator_ref<std::allocator<char>, 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<std::allocator<char>, false>;
+
+TEST(memory_buffer_test, move_ctor_inline_buffer_non_propagating) {
+  auto check_move_buffer =
+      [](const char* str,
+         basic_memory_buffer<char, 5, std_allocator_n>& buffer) {
+        std::allocator<char>* original_alloc_ptr = buffer.get_allocator().get();
+        const char* original_data_ptr = &buffer[0];
+        basic_memory_buffer<char, 5, std_allocator_n> 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<char>();
+  basic_memory_buffer<char, 5, std_allocator_n> 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<char>();
+  basic_memory_buffer<char, 4, std_allocator_n> 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<char>* original_alloc_ptr = buffer.get_allocator().get();
+  basic_memory_buffer<char, 4, std_allocator_n> 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<char, 5>& buffer) {
   basic_memory_buffer<char, 5> buffer2;

test/mock-allocator.h

@@ -37,7 +37,8 @@ template <typename T> class mock_allocator {
   MOCK_METHOD(void, deallocate, (T*, size_t));
 };
 
-template <typename Allocator> class allocator_ref {
+template <typename Allocator, bool PropagateOnMove = false>
+class allocator_ref {
  private:
   Allocator* alloc_;
 
@@ -48,6 +49,9 @@ template <typename Allocator> class allocator_ref {
 
  public:
   using value_type = typename Allocator::value_type;
+  using propagate_on_container_move_assignment =
+      typename std::conditional<PropagateOnMove, std::true_type,
+                                std::false_type>::type;
 
   explicit allocator_ref(Allocator* alloc = nullptr) : alloc_(alloc) {}
 
@@ -72,6 +76,19 @@ template <typename Allocator> class allocator_ref {
     return std::allocator_traits<Allocator>::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_