From b3aa763eb8b086ec0c5fac1ed9ef6d526e3bd198 Mon Sep 17 00:00:00 2001 From: Domagoj Saric Date: Thu, 19 Dec 2024 14:56:46 +0100 Subject: [PATCH 1/2] Fixed bugs in vector_impl<>::make_space_for_insert(). Added matching tests. --- include/psi/vm/containers/vector_impl.hpp | 22 +++++++++++++--- test/vector.cpp | 32 ++++++++++++++--------- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/include/psi/vm/containers/vector_impl.hpp b/include/psi/vm/containers/vector_impl.hpp index feb8de1..b0ca9f8 100644 --- a/include/psi/vm/containers/vector_impl.hpp +++ b/include/psi/vm/containers/vector_impl.hpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -858,14 +859,27 @@ class [[ nodiscard, clang::trivial_abi ]] vector_impl auto const elements_to_move{ static_cast( current_size - position_index ) }; if constexpr ( is_trivially_moveable ) { - std::uninitialized_move_n( &data[ position_index ], elements_to_move, &data[ position_index + n ] ); + // does not use is_trivially_moveable/trivial_abi and is incorrect + // (i.e. an uninitialized_move_backwards is required) + //std::uninitialized_move_n( &data[ position_index ], elements_to_move, &data[ position_index + n ] ); + std::memmove( &data[ position_index + n ], &data[ position_index ], elements_to_move * sizeof( *data ) ); } - else // future support for generic types + else { auto const elements_to_move_to_uninitialized_space{ n }; auto const elements_to_move_to_the_current_end { static_cast( elements_to_move - elements_to_move_to_uninitialized_space ) }; - std::uninitialized_move_n( &data[ current_size - elements_to_move_to_uninitialized_space ], elements_to_move_to_uninitialized_space, &data[ current_size ] ); - std::move ( &data[ position_index ], &data[ position_index + elements_to_move_to_the_current_end ], &data[ position_index + n ] ); + std::uninitialized_move + ( + &data[ current_size - elements_to_move_to_uninitialized_space ], + &data[ current_size ], + &data[ current_size ] + ); + std::move_backward + ( + &data[ position_index ], + &data[ position_index + elements_to_move_to_the_current_end ], + &data[ position_index + elements_to_move_to_the_current_end + n ] + ); } return self.make_iterator( &data[ position_index ] ); } diff --git a/test/vector.cpp b/test/vector.cpp index e0d6a1a..ec8bbd9 100644 --- a/test/vector.cpp +++ b/test/vector.cpp @@ -44,33 +44,41 @@ TEST(vector_test, element_access) { TEST(vector_test, modifiers) { - crt_vector vec; + using test_str_t = std::conditional_t, std::string, std::string_view>; + crt_vector vec; // Test push_back - vec.push_back(1); - vec.push_back(2); + vec.emplace_back("1"); + vec.emplace_back("2"); EXPECT_EQ(vec.size(), 2); - EXPECT_EQ(vec[1], 2); + EXPECT_EQ(vec[1], "2"); // Test emplace_back - vec.emplace_back(3); + vec.emplace_back("3"); EXPECT_EQ(vec.size(), 3); - EXPECT_EQ(vec[2], 3); + EXPECT_EQ(vec[2], "3"); // Test pop_back vec.pop_back(); EXPECT_EQ(vec.size(), 2); - EXPECT_EQ(vec.back(), 2); + EXPECT_EQ(vec.back(), "2"); // Test insert - vec.insert(vec.begin(), 0); - EXPECT_EQ(vec.front(), 0); - EXPECT_EQ(vec.size(), 3); + std::string_view const sbo_overflower{ "01234567898765432100123456789876543210" }; + vec.emplace( vec.end () , sbo_overflower ); + vec.emplace( vec.begin() , "0" ); + vec.emplace( vec.nth( 3 ), "3" ); + EXPECT_EQ( vec.front(), "0" ); + EXPECT_EQ( vec[ 1 ] , "1" ); + EXPECT_EQ( vec[ 2 ] , "2" ); + EXPECT_EQ( vec[ 3 ] , "3" ); + EXPECT_EQ( vec[ 4 ] , sbo_overflower ); + EXPECT_EQ( vec.size(), 5 ); // Test erase vec.erase(vec.begin()); - EXPECT_EQ(vec.front(), 1); - EXPECT_EQ(vec.size(), 2); + EXPECT_EQ(vec.front(), "1"); + EXPECT_EQ(vec.size(), 4); // Test clear vec.clear(); From fdd56731599b5e1280acad2907024f26bf8f7a20 Mon Sep 17 00:00:00 2001 From: Domagoj Saric Date: Thu, 19 Dec 2024 17:30:18 +0100 Subject: [PATCH 2/2] Fixed double element destruction in crt_vector. --- include/psi/vm/containers/crt_vector.hpp | 17 ++++++++--------- include/psi/vm/containers/static_vector.hpp | 6 ++++-- include/psi/vm/containers/vector_impl.hpp | 2 +- include/psi/vm/containers/vm_vector.hpp | 7 ++++--- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/include/psi/vm/containers/crt_vector.hpp b/include/psi/vm/containers/crt_vector.hpp index e9ce998..ea677c3 100644 --- a/include/psi/vm/containers/crt_vector.hpp +++ b/include/psi/vm/containers/crt_vector.hpp @@ -407,10 +407,10 @@ class [[ nodiscard, clang::trivial_abi ]] crt_vector std::swap( this->p_array_ , other.p_array_ ); std::swap( this->size_ , other.size_ ); std::swap( this->capacity_, other.capacity_ ); - other.free(); + other.clear(); return *this; } - constexpr ~crt_vector() noexcept { free(); } + constexpr ~crt_vector() noexcept { base::clear(); } [[ nodiscard, gnu::pure ]] size_type size () const noexcept { return size_; } [[ nodiscard, gnu::pure ]] size_type capacity() const noexcept @@ -485,6 +485,12 @@ private: friend base; void storage_dec_size() noexcept { BOOST_ASSUME( size_ >= 1 ); --size_; } void storage_inc_size() noexcept; // TODO + void storage_free() noexcept + { + al::deallocate( data(), capacity() ); + mark_freed(); + } + private: [[ gnu::cold, gnu::noinline, clang::preserve_most ]] void do_grow( size_type const target_size, size_type const cached_current_capacity ) @@ -514,13 +520,6 @@ private: friend base; } } - void free() noexcept - { - std::destroy_n( data(), size() ); - al::deallocate( data(), capacity() ); - mark_freed(); - } - void mark_freed() noexcept { std::memset( this, 0, sizeof( *this ) ); } private: diff --git a/include/psi/vm/containers/static_vector.hpp b/include/psi/vm/containers/static_vector.hpp index 5020752..8538f24 100644 --- a/include/psi/vm/containers/static_vector.hpp +++ b/include/psi/vm/containers/static_vector.hpp @@ -151,8 +151,10 @@ private: friend base; BOOST_ASSUME( size_ >= target_size ); size_ = target_size; } - void storage_dec_size() noexcept { BOOST_ASSUME( size_ >= 1 ); --size_; } - void storage_inc_size() noexcept; // TODO + constexpr void storage_dec_size() noexcept { BOOST_ASSUME( size_ >= 1 ); --size_; } + constexpr void storage_inc_size() noexcept; // TODO + + constexpr void storage_free() noexcept { size_ = 0; } private: void fixed_copy( static_vector const & __restrict source ) noexcept diff --git a/include/psi/vm/containers/vector_impl.hpp b/include/psi/vm/containers/vector_impl.hpp index b0ca9f8..975b5c4 100644 --- a/include/psi/vm/containers/vector_impl.hpp +++ b/include/psi/vm/containers/vector_impl.hpp @@ -757,7 +757,7 @@ class [[ nodiscard, clang::trivial_abi ]] vector_impl void clear( this Impl & self ) noexcept { std::destroy( self.begin(), self.end() ); - self.free(); + self.storage_free(); } void swap( this auto & self, auto & other ) noexcept { std::swap( self, other ); } diff --git a/include/psi/vm/containers/vm_vector.hpp b/include/psi/vm/containers/vm_vector.hpp index 802afcf..3ca9fdd 100644 --- a/include/psi/vm/containers/vm_vector.hpp +++ b/include/psi/vm/containers/vm_vector.hpp @@ -364,13 +364,15 @@ private: friend vec_impl; //! n; otherwise, capacity() is unchanged. In either case, size() is unchanged. //! //! Throws: If memory allocation allocation throws or T's copy/move constructor throws. - T * storage_shrink_to( sz_t const target_size ) noexcept { return static_cast( base::shrink_to( to_byte_sz( target_size ) ) ); } T * storage_grow_to ( sz_t const target_size ) { return static_cast( base::grow_to ( to_byte_sz( target_size ) ) ); } + T * storage_shrink_to( sz_t const target_size ) noexcept { return static_cast( base::shrink_to( to_byte_sz( target_size ) ) ); } void storage_shrink_size_to( sz_t const new_size ) noexcept { base::shrink_size_to( to_byte_sz( new_size ) ); } void storage_dec_size() noexcept { storage_shrink_size_to( size() - 1 ); } void storage_inc_size() noexcept; // TODO + void storage_free() noexcept { base::free(); } + protected: PSI_WARNING_DISABLE_PUSH() PSI_WARNING_GCC_OR_CLANG_DISABLE( -Wsign-conversion ) @@ -471,8 +473,7 @@ class [[ clang::trivial_abi ]] vm_vector { private: using storage_t = typed_contiguous_storage; - //using impl = vector_impl; - using impl = storage_t; + using impl = storage_t; public: static constexpr auto headerless{ headerless_param };