Fixed bugs in vector_impl<>::make_space_for_insert().

Added matching tests.
psiha · Dec 19, 2024 · e3ebb97 · e3ebb97
1 parent 9471209
commit e3ebb97
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 35 deletions.
diff --git a/include/psi/vm/containers/vector_impl.hpp b/include/psi/vm/containers/vector_impl.hpp
@@ -39,6 +39,7 @@
 #include <cstring>
 #include <iterator>
 #include <memory>
+#include <ranges>
 #include <span>
 #include <type_traits>
 #include <std_fix/const_iterator.hpp>
@@ -858,14 +859,27 @@ class [[ nodiscard, clang::trivial_abi ]] vector_impl
         auto const elements_to_move{ static_cast<size_type>( current_size - position_index ) };
         if constexpr ( is_trivially_moveable<value_type> )
         {
-            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<size_type>( 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
@@ -44,37 +44,87 @@ TEST(vector_test, element_access) {
 
 
 TEST(vector_test, modifiers) {
-    crt_vector<int> vec;
-
-    // Test push_back
-    vec.push_back(1);
-    vec.push_back(2);
-    EXPECT_EQ(vec.size(), 2);
-    EXPECT_EQ(vec[1], 2);
-
-    // Test emplace_back
-    vec.emplace_back(3);
-    EXPECT_EQ(vec.size(), 3);
-    EXPECT_EQ(vec[2], 3);
-
-    // Test pop_back
-    vec.pop_back();
-    EXPECT_EQ(vec.size(), 2);
-    EXPECT_EQ(vec.back(), 2);
-
-    // Test insert
-    vec.insert(vec.begin(), 0);
-    EXPECT_EQ(vec.front(), 0);
-    EXPECT_EQ(vec.size(), 3);
-
-    // Test erase
-    vec.erase(vec.begin());
-    EXPECT_EQ(vec.front(), 1);
-    EXPECT_EQ(vec.size(), 2);
-
-    // Test clear
-    vec.clear();
-    EXPECT_TRUE(vec.empty());
+    if constexpr ( is_trivially_moveable<std::string> )
+    {
+        crt_vector<std::string> vec;
+
+        // Test push_back
+        vec.emplace_back("1");
+        vec.emplace_back("2");
+        EXPECT_EQ(vec.size(), 2);
+        EXPECT_EQ(vec[1], "2");
+
+        // Test emplace_back
+        vec.emplace_back("3");
+        EXPECT_EQ(vec.size(), 3);
+        EXPECT_EQ(vec[2], "3");
+
+        // Test pop_back
+        vec.pop_back();
+        EXPECT_EQ(vec.size(), 2);
+        EXPECT_EQ(vec.back(), "2");
+
+        // Test insert
+        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(), 4);
+
+        // Test clear
+        vec.clear();
+        EXPECT_TRUE(vec.empty());
+    }
+    else
+    {
+        crt_vector<int> vec;
+
+        // Test push_back
+        vec.push_back( 1 );
+        vec.push_back( 2 );
+        EXPECT_EQ( vec.size(), 2 );
+        EXPECT_EQ( vec[ 1 ], 2 );
+
+        // Test emplace_back
+        vec.emplace_back( 3 );
+        EXPECT_EQ( vec.size(), 3 );
+        EXPECT_EQ( vec[ 2 ], 3 );
+
+        // Test pop_back
+        vec.pop_back();
+        EXPECT_EQ( vec.size(), 2 );
+        EXPECT_EQ( vec.back(), 2 );
+
+        // Test insert
+        vec.emplace( vec.end  () , INT_MAX );
+        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 ]   , INT_MAX );
+        EXPECT_EQ( vec.size(), 5 );
+
+        // Test erase
+        vec.erase( vec.begin() );
+        EXPECT_EQ( vec.front(), 1 );
+        EXPECT_EQ( vec.size(), 4 );
+
+        // Test clear
+        vec.clear();
+        EXPECT_TRUE( vec.empty() );
+    }
 }