Handle allocator propagation in basic_memory_buffer::move, Fix #4487 #4490

toprakmurat · 2025-07-08T11:15:57Z

This PR implements allocator-aware move behavior in basic_memory_buffer, following the suggestion by @dlex in the linked issue.

To enable allocator comparison (alloc_ == other.alloc_), I added operator== and operator!= to the following classes:

Summary of Changes

Move logic now respects std::allocator_traits<>::propagate_on_container_move_assignment.
If allocator propagation is disabled and the allocators differ, the buffer content is copied instead of moved.

Test Failures

There are currently two failing tests (Test 7/21) that may need to be updated to match the new move semantics:

move_ctor_inline_buffer

// Move should transfer allocator.
EXPECT_EQ(nullptr, buffer.get_allocator().get());
EXPECT_EQ(alloc, buffer2.get_allocator().get());

move_ctor_dynamic_buffer

// Move should rip the guts of the first buffer.
EXPECT_EQ(&buffer[0], inline_buffer_ptr);
EXPECT_EQ(buffer.size(), 0);

I propose updating the above tests to align with the allocator propagation semantics. However, before proceeding, I’d like to get feedback from maintainers and reviewers. @vitaut

Do these changes and the proposed test adjustments align with the intended design of basic_memory_buffer?

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.

dlex

Looks very good, definitely a move in the right direction. Please see my comments about C++11 compatibility, and a couple about more optimal buffer copying.

dlex · 2025-07-08T15:39:25Z

include/fmt/format.h

    T* data = other.data();
    size_t size = other.size(), capacity = other.capacity();
+    if constexpr (alloc_traits::propagate_on_container_move_assignment::value) {


if constexpr is a C++17 feature, and {fmt} compatibility depth is C++11, I believe. However std::pmr::polymorphic_allocator (which is the primary case for the non-propagating allocators problem) is a C++17 feature as well. So I think it's ok to limit this entire new code block to C++17 (__cpp_if_constexpr might have been the right feature guard to check, but I'm concerned of older C++17 compilers not supporting feature guards yet).

It's better to avoid conditional compilation if possible. Can we just use if here?

I guess if will have the same issue as reported in #4487.

Can we just use if here?

Nope, it will require move assignment operator from the allocator to compile in all cases then.

I guess if will have the same issue as reported in #4487.

Exactly.

Another option is to fall back to the old school SFINAE monsters, and conditional compilation looks like a lesser evil to me. @vitaut WDYT?

I recommend using compiler-specific if constexpr capability detection, as SFINAE introduces unnecessary logical complexity and a regular if statement won't compile in this context.

How about adding this snippet to base.h?

// Detect C++17 if constexpr support #ifdef FMT_USE_CONSTEXPR17 // Use the provided definition. #elif defined(__cpp_if_constexpr) && __cpp_if_constexpr >= 201606L // Standard feature test macro for if constexpr # define FMT_USE_CONSTEXPR17 1 #elif FMT_GCC_VERSION >= 700 && FMT_CPLUSPLUS >= 201703L # define FMT_USE_CONSTEXPR17 1 // GCC 7.0+ with C++17 mode #elif FMT_CLANG_VERSION >= 309 && FMT_CPLUSPLUS >= 201703L # define FMT_USE_CONSTEXPR17 1 // Clang 3.9+ with C++17 mode #elif FMT_MSC_VERSION >= 1911 && FMT_CPLUSPLUS >= 201703L # define FMT_USE_CONSTEXPR17 1 // MSVC 2017 15.3+ with C++17 mode #elif FMT_ICC_VERSION >= 1810 && FMT_CPLUSPLUS >= 201703L # define FMT_USE_CONSTEXPR17 1 // Intel C++ 18.1+ with C++17 mode #elif FMT_HAS_FEATURE(cxx_if_constexpr) # define FMT_USE_CONSTEXPR17 1 // Clang feature detection #else # define FMT_USE_CONSTEXPR17 0 #endif #if FMT_USE_CONSTEXPR17 # define FMT_CONSTEXPR17 if constexpr #else # define FMT_CONSTEXPR17 if #endif

source: cppreference

I think SFINAE shouldn't be too complex, definitely better than introducing FMT_USE_CONSTEXPR17.

dlex · 2025-07-08T15:52:22Z

include/fmt/format.h

+      alloc_ = std::move(other.alloc_);
+    } else {
+      if (alloc_ != other.alloc_) {
+        this->reserve(capacity);


It might be costly to propagate the entire capacity when copying. What if the source buffer used to be 10k, but then shrunk in size to 1k. Its capacity is 10k still. But is there any point in allocating 10k at the target when the actual data to be copied is 1k?

dlex · 2025-07-08T15:56:32Z

include/fmt/format.h

+        this->reserve(capacity);
+        detail::copy<T>(data, data + size, this->data());
+        this->resize(size);
+        return;


This branch needs to handle the case when data == other.store_ as well. Otherwise the store_ buffer optimization is bypassed for the target buffer.

You're right.

Also, do we need to clear other if data != other.store_?

i.e.

other.set(other.store_, 0); other.clear();

I would say no. Nothing in the standard requires move assignment operator argument to be in a certain state after a successful call, empty or whatever.

vitaut · 2025-07-08T18:36:42Z

include/fmt/format.h

@@ -746,6 +746,14 @@ template <typename T> struct allocator : private std::decay<void> {
  }

  void deallocate(T* p, size_t) { std::free(p); }
+
+  friend bool operator==(const allocator&, const allocator&) noexcept {


Let's pass allocator by value.

vitaut

Thanks for the PR. Please add a test case and address inline comments.

vitaut · 2025-07-09T18:35:48Z

include/fmt/format.h

@@ -747,12 +747,12 @@ template <typename T> struct allocator : private std::decay<void> {

  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, const allocator) noexcept {


const is not needed here and below

- 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

vitaut · 2025-07-13T17:03:52Z

test/format-test.cc

+  buffer.push_back('a');
+  check_move_buffer("testa", buffer);


How is this different from the first check? Can we have a single check and get rid of the lambda?

vitaut · 2025-07-13T17:05:05Z

include/fmt/format.h

  // 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<Allocator>;


Looks unused.

toprakmurat mentioned this pull request Jul 8, 2025

fmt::basic_memory_buffer<...,std::pmr::polymorphic_allocator> is not moveable #4487

Open

dlex reviewed Jul 8, 2025

View reviewed changes

vitaut reviewed Jul 8, 2025

View reviewed changes

vitaut requested changes Jul 8, 2025

View reviewed changes

toprakmurat marked this pull request as draft July 9, 2025 15:11

vitaut reviewed Jul 9, 2025

View reviewed changes

toprakmurat added 2 commits July 12, 2025 00:28

Update move ctor to be C++11 compatible and minor fixes

d130e5d

toprakmurat force-pushed the fix/issue-4487 branch from 35b131e to 764fca9 Compare July 11, 2025 21:41

toprakmurat marked this pull request as ready for review July 11, 2025 21:43

toprakmurat requested review from vitaut and dlex July 12, 2025 17:22

vitaut reviewed Jul 13, 2025

View reviewed changes

Handle allocator propagation in basic_memory_buffer::move, Fix #4487 #4490

Are you sure you want to change the base?

Handle allocator propagation in basic_memory_buffer::move, Fix #4487 #4490

Conversation

toprakmurat commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Test Failures

Uh oh!

dlex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitaut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toprakmurat commented Jul 8, 2025 •

edited

Loading