Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vitaut merged 7 commits into from
Aug 9, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ce2f6b8
Handle allocator propagation in basic_memory_buffer::move
toprakmurat Jul 8, 2025
49f8b4c
Update move ctor to be C++11 compatible and minor fixes
toprakmurat Jul 11, 2025
7e8a9b9
Add test cases for the updated move ctor
toprakmurat Jul 11, 2025
fe8d8b6
fix resizing error in allocator_move_impl
toprakmurat Jul 31, 2025
55e1358
update move ctor tests
toprakmurat Aug 1, 2025
4e219ee
remove unused variable and type alias
toprakmurat Aug 3, 2025
5cd0e40
add constexpr support for the allocator_move_impl
toprakmurat Aug 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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_);
Expand Down
52 changes: 51 additions & 1 deletion test/format-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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()));
Copy link

@dlex dlex Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU, this check is here to verify that the underlying memory buffer have not been moved from buffer to buffer2, and it's used to verify that memory copying has happened instead. You may want to consider a more black box way to test that: compare a copy of &buffer[0] before the move, and &buffer2[0] after the move, they should be different. And vice versa: where we should expect the underlying buffer has been moved, they should be the same.

Copy link
Contributor Author

@toprakmurat toprakmurat Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you suggest is a great idea, but I also believe it's essential to verify the contents as well. Since we're copying, it's crucial to ensure the contents remain the same. That said, I will apply your recommendation by adding:

EXPECT_NE(original_data_ptr, new_data_ptr);

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');
Copy link

@dlex dlex Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer has just been moved-from in the 1st invocation of check_move_buffer. I think the test code should not expect that the original data have left in it. It may be better to reassign buffer completely, or use another instance of basic_memory_buffer for the following test.

Copy link
Contributor Author

@toprakmurat toprakmurat Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test you're referring to specifically targets the inline buffer, and for that reason, the buffer should remain unchanged. Its purpose is to verify that the content stays the same and that no heap allocation occurs as long as it's within the capacity limit. If you take a look at the move implementation in format.h, you'll see that when an inline buffer is used, the content is simply copied.

Copy link

@dlex dlex Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand that buffer sized under small buffer optimizaion threshold is simply copied, but that's not the question.

The question is the same as in the line 366 comment thread above. Which essentially is: is the fact that basic_memory_buffer's buffer keeps the data smaller than the SBO threshold after being moved-from a part of its interface guarantee, or is it an implementation detail?

If it's a part of interface, both these checks are correct and even required.

If it's not, we are testing implementation detail assumptions here which don't have to hold, even though the checks are momentarily correct.

There is no doc about whether it's an interface guarantee, or a comment in the code, that's why I assume the latter. @vitaut has the ultimate say here however.

Copy link
Contributor

@vitaut vitaut Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlex made a good point.

is the fact that basic_memory_buffer's buffer keeps the data smaller than the SBO threshold after being moved-from a part of its interface guarantee, or is it an implementation detail?

It is definitely an implementation detail and we shouldn't rely on it here.

Copy link
Contributor

@vitaut vitaut Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this can be done as a follow-up.

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;
Expand Down
19 changes: 18 additions & 1 deletion test/mock-allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;

Expand All @@ -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) {}

Expand All @@ -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_
Loading