diff --git a/include/xtensor/containers/xstorage.hpp b/include/xtensor/containers/xstorage.hpp index 26d4a8ccd..3f4b43027 100644 --- a/include/xtensor/containers/xstorage.hpp +++ b/include/xtensor/containers/xstorage.hpp @@ -897,12 +897,15 @@ namespace xt template inline void svector::assign(size_type n, const value_type& v) { + // v may refer to an element of this vector, and grow() may free the + // buffer that holds it. + const value_type value(v); if (n > N && n > capacity()) { grow(n); } m_end = m_begin + n; - std::fill(begin(), end(), v); + std::fill(begin(), end(), value); } template @@ -1022,9 +1025,16 @@ namespace xt { if (m_end >= m_capacity) { + // elt may refer to an element of this vector, and grow() may free + // the buffer that holds it. + T value(elt); grow(); + *(m_end++) = std::move(value); + } + else + { + *(m_end++) = elt; } - *(m_end++) = elt; } template @@ -1032,9 +1042,15 @@ namespace xt { if (m_end >= m_capacity) { + // Same as above: take the value out before grow() runs. + T value(std::move(elt)); grow(); + *(m_end++) = std::move(value); + } + else + { + *(m_end++) = std::move(elt); } - *(m_end++) = std::move(elt); } template @@ -1196,13 +1212,10 @@ namespace xt template inline auto svector::insert(const_iterator cit, const T& elt) -> iterator { + // elt may refer to an element of this vector: the shift below moves it + // and grow() may free the buffer that holds it. + T value(elt); auto it = const_cast(cit); - if (it == m_end) - { - push_back(elt); - return m_end - 1; - } - if (m_end >= m_capacity) { std::ptrdiff_t elt_no = it - m_begin; @@ -1210,17 +1223,9 @@ namespace xt it = m_begin + elt_no; } - (*m_end) = back(); - std::move_backward(it, m_end - 1, m_end); + std::move_backward(it, m_end, m_end + 1); ++m_end; - - // Update ref if element moved - const T* elt_ptr = &elt; - bool cond = it <= elt_ptr && elt_ptr < m_end; - // More complicated than incrementing elt_ptr, but this avoids - // false positive array-bounds warning on GCC 10 - const T* src_ptr = cond ? it + (elt_ptr - it) + std::ptrdiff_t(1) : elt_ptr; - *it = *src_ptr; + *it = std::move(value); return it; } diff --git a/test/test_xstorage.cpp b/test/test_xstorage.cpp index cb66a8c69..8c9feb870 100644 --- a/test/test_xstorage.cpp +++ b/test/test_xstorage.cpp @@ -145,6 +145,69 @@ namespace xt EXPECT_TRUE(std::equal(s.begin(), s.end(), v.begin())); } + // A full heap buffer: the next insertion reallocates and frees the old + // buffer, the one that holds the aliased argument in the tests below. + using small_svector_type = svector; + + TEST(svector, insert_aliased_element) + { + { + small_svector_type s = {1, 2, 3}; + ASSERT_FALSE(s.on_stack()); + ASSERT_EQ(s.size(), s.capacity()); + s.insert(s.begin(), s[0]); + EXPECT_EQ(s, small_svector_type({1, 1, 2, 3})); + } + + { + small_svector_type s = {1, 2, 3}; + ASSERT_EQ(s.size(), s.capacity()); + s.insert(s.begin() + 1, s[1]); + EXPECT_EQ(s, small_svector_type({1, 2, 2, 3})); + } + + { + small_svector_type s = {1, 2, 3}; + ASSERT_EQ(s.size(), s.capacity()); + s.insert(s.end(), s[0]); + EXPECT_EQ(s, small_svector_type({1, 2, 3, 1})); + } + + { + // No reallocation: the shift moves the aliased element. + small_svector_type s = {1, 2, 3}; + s.reserve(8); + s.insert(s.begin(), s[1]); + EXPECT_EQ(s, small_svector_type({2, 1, 2, 3})); + } + } + + TEST(svector, push_back_aliased_element) + { + { + small_svector_type s = {1, 2, 3}; + ASSERT_FALSE(s.on_stack()); + ASSERT_EQ(s.size(), s.capacity()); + s.push_back(s[0]); + EXPECT_EQ(s, small_svector_type({1, 2, 3, 1})); + } + + { + small_svector_type s = {1, 2, 3}; + ASSERT_EQ(s.size(), s.capacity()); + s.push_back(std::move(s[1])); + EXPECT_EQ(s, small_svector_type({1, 2, 3, 2})); + } + } + + TEST(svector, assign_aliased_element) + { + small_svector_type s = {7, 2, 3}; + ASSERT_FALSE(s.on_stack()); + s.assign(5, s[0]); + EXPECT_EQ(s, small_svector_type({7, 7, 7, 7, 7})); + } + TEST(svector, insert_range) { svector_type s0 = {1, 2, 3, 4};