From c92fe75e15d55f955e3de81f0ba761a1644066c4 Mon Sep 17 00:00:00 2001 From: Loic Gouarin Date: Thu, 29 Oct 2026 17:54:01 +0100 Subject: [PATCH] Make svector moves and swap non-allocating The svector move constructor and move assignment are noexcept, but the constructor swapped with an empty vector and the assignment copied the elements. When the source held more than N elements, both allocated a new heap buffer, so a std::bad_alloc called std::terminate. Every move of an xarray with more than four dimensions went through that path. The noexcept free swap also called the member swap, which can resize. Moving now takes over the heap buffer of the source, together with its allocator. When the source uses its inline buffer, the elements are moved into the current storage, which always has room for N elements. The moved-from vector is left empty. Swapping two svectors of the same type goes through three moves and does not allocate either. The noexcept conditions now use is_nothrow_move_assignable, which matches how the elements are transferred. --- include/xtensor/containers/xstorage.hpp | 120 +++++++++++++++++------- test/test_xstorage.cpp | 39 ++++++++ 2 files changed, 124 insertions(+), 35 deletions(-) diff --git a/include/xtensor/containers/xstorage.hpp b/include/xtensor/containers/xstorage.hpp index 26d4a8ccd..bb74034f9 100644 --- a/include/xtensor/containers/xstorage.hpp +++ b/include/xtensor/containers/xstorage.hpp @@ -680,7 +680,7 @@ namespace xt requires(N != N2); svector(const svector& other); - svector(svector&& other) noexcept(std::is_nothrow_move_constructible::value); + svector(svector&& other) noexcept(std::is_nothrow_move_assignable::value); void assign(size_type n, const value_type& v); @@ -761,6 +761,7 @@ namespace xt void grow(size_type min_capacity = 0); void destroy_range(T* begin, T* end); + void move_from(svector& rhs) noexcept(std::is_nothrow_move_assignable::value); }; template @@ -846,7 +847,10 @@ namespace xt inline svector& svector::operator=(svector&& rhs ) noexcept(std::is_nothrow_move_assignable::value) { - assign(rhs.begin(), rhs.end()); + if (this != &rhs) + { + move_from(rhs); + } return *this; } @@ -889,9 +893,10 @@ namespace xt template inline svector::svector(svector&& rhs - ) noexcept(std::is_nothrow_move_constructible::value) + ) noexcept(std::is_nothrow_move_assignable::value) + : m_allocator(rhs.m_allocator) { - this->swap(rhs); + move_from(rhs); } template @@ -1275,46 +1280,89 @@ namespace xt return; } - // We can only avoid copying elements if neither vector is small. - if (!this->on_stack() && !rhs.on_stack()) + if constexpr (std::is_same, self_type>::value) { - swap(this->m_begin, rhs.m_begin); - swap(this->m_end, rhs.m_end); - swap(this->m_capacity, rhs.m_capacity); - return; + // Moves never allocate, so swapping two vectors of the same type cannot fail. + self_type tmp(std::move(*this)); + *this = std::move(rhs); + rhs = std::move(tmp); } + else + { + // We can only avoid copying elements if neither vector is small. + if (!this->on_stack() && !rhs.on_stack()) + { + swap(this->m_begin, rhs.m_begin); + swap(this->m_end, rhs.m_end); + swap(this->m_capacity, rhs.m_capacity); + return; + } - size_type rhs_old_size = rhs.size(); - size_type old_size = this->size(); + size_type rhs_old_size = rhs.size(); + size_type old_size = this->size(); - if (rhs_old_size > old_size) - { - this->resize(rhs_old_size); - } - else if (old_size > rhs_old_size) - { - rhs.resize(old_size); - } + if (rhs_old_size > old_size) + { + this->resize(rhs_old_size); + } + else if (old_size > rhs_old_size) + { + rhs.resize(old_size); + } - // Swap the shared elements. - size_type min_size = (std::min)(old_size, rhs_old_size); - for (size_type i = 0; i < min_size; ++i) - { - swap((*this)[i], rhs[i]); + // Swap the shared elements. + size_type min_size = (std::min)(old_size, rhs_old_size); + for (size_type i = 0; i < min_size; ++i) + { + swap((*this)[i], rhs[i]); + } + + // Copy over the extra elts. + if (old_size > rhs_old_size) + { + std::copy(this->begin() + min_size, this->end(), rhs.begin() + min_size); + this->destroy_range(this->begin() + min_size, this->end()); + this->m_end = this->begin() + min_size; + } + else if (rhs_old_size > old_size) + { + std::copy(rhs.begin() + min_size, rhs.end(), this->begin() + min_size); + this->destroy_range(rhs.begin() + min_size, rhs.end()); + rhs.m_end = rhs.begin() + min_size; + } } + } - // Copy over the extra elts. - if (old_size > rhs_old_size) + template + inline void svector::move_from(svector& rhs + ) noexcept(std::is_nothrow_move_assignable::value) + { + if (rhs.on_stack()) { - std::copy(this->begin() + min_size, this->end(), rhs.begin() + min_size); - this->destroy_range(this->begin() + min_size, this->end()); - this->m_end = this->begin() + min_size; + // rhs holds at most N elements, and the current storage (inline buffer or a heap + // buffer, whose capacity is always greater than N) can hold them. + m_end = std::move(rhs.m_begin, rhs.m_end, m_begin); + rhs.m_end = rhs.m_begin; } - else if (rhs_old_size > old_size) + else { - std::copy(rhs.begin() + min_size, rhs.end(), this->begin() + min_size); - this->destroy_range(rhs.begin() + min_size, rhs.end()); - rhs.m_end = rhs.begin() + min_size; + // Take ownership of the heap buffer of rhs together with the allocator that + // allocated it. + if (!on_stack()) + { + detail::safe_destroy_deallocate( + m_allocator, + m_begin, + static_cast(m_capacity - m_begin) + ); + } + m_allocator = rhs.m_allocator; + m_begin = rhs.m_begin; + m_end = rhs.m_end; + m_capacity = rhs.m_capacity; + rhs.m_begin = std::begin(rhs.m_data); + rhs.m_end = std::begin(rhs.m_data); + rhs.m_capacity = std::end(rhs.m_data); } } @@ -1398,7 +1446,9 @@ namespace xt } template - inline void swap(svector& lhs, svector& rhs) noexcept + inline void + swap(svector& lhs, svector& rhs) noexcept(std::is_nothrow_move_assignable::value + ) { lhs.swap(rhs); } diff --git a/test/test_xstorage.cpp b/test/test_xstorage.cpp index cb66a8c69..1ffe78af4 100644 --- a/test/test_xstorage.cpp +++ b/test/test_xstorage.cpp @@ -8,6 +8,8 @@ ****************************************************************************/ #include +#include +#include #include "xtensor/containers/xstorage.hpp" #include "xtensor/core/xtensor_config.hpp" @@ -345,4 +347,41 @@ namespace xt EXPECT_EQ(a.front(), size_t(3)); EXPECT_EQ(a.size(), size_t(3)); } + + TEST(svector, move) + { + static_assert(std::is_nothrow_move_constructible::value); + static_assert(std::is_nothrow_move_assignable::value); + static_assert(std::is_nothrow_swappable::value); + + // A heap buffer is transferred, not copied. + svector_type a = {1, 3, 4, 6, 7}; + const std::size_t* heap = a.data(); + svector_type b(std::move(a)); + EXPECT_EQ(b.data(), heap); + EXPECT_EQ(b, svector_type({1, 3, 4, 6, 7})); + EXPECT_TRUE(a.empty()); + + svector_type c = {1, 2}; + c = std::move(b); + EXPECT_EQ(c.data(), heap); + EXPECT_EQ(c, svector_type({1, 3, 4, 6, 7})); + EXPECT_TRUE(b.empty()); + + // Elements stored inline are moved into the existing storage. + svector_type d = {5, 6}; + svector_type e(std::move(d)); + EXPECT_EQ(e, svector_type({5, 6})); + EXPECT_TRUE(d.empty()); + + c = std::move(e); + EXPECT_EQ(c.data(), heap); + EXPECT_EQ(c, svector_type({5, 6})); + EXPECT_TRUE(e.empty()); + + // Moved-from vectors can be reused. + a = {8, 9}; + EXPECT_EQ(a, svector_type({8, 9})); + } + }