Fix bptree copy cstr/assignment (#119)

tzaeschke · Feb 9, 2023 · d3d98f6 · d3d98f6
1 parent 56e0f2a
commit d3d98f6
Show file tree

Hide file tree

Showing 9 changed files with 276 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Removed
 - bazel version requirement file `.bazelversion`. [#89](https://github.com/tzaeschke/phtree-cpp/issues/89)
 
+###
+- Fixed copy cstr/assignment of B+trees, see also #102. [#119](https://github.com/tzaeschke/phtree-cpp/pull/119) 
+
 ## [1.4.0] - 2022-09-09
 ### Added
 - Added build features: [#53](https://github.com/tzaeschke/phtree-cpp/issues/53) 

diff --git a/include/phtree/common/b_plus_tree_hash_map.h b/include/phtree/common/b_plus_tree_hash_map.h
@@ -96,8 +96,8 @@ class b_plus_tree_hash_set {
     explicit b_plus_tree_hash_set() : root_{new NLeafT(nullptr, nullptr, nullptr)}, size_{0} {};
 
     b_plus_tree_hash_set(const b_plus_tree_hash_set& other) : size_{other.size_} {
-        root_ = other.root_->is_leaf() ? new NLeafT(*other.root_->as_leaf())
-                                       : new NInnerT(*other.root_->as_inner());
+        root_ = other.root_->is_leaf() ? (NodeT*)new NLeafT(*other.root_->as_leaf())
+                                       : (NodeT*)new NInnerT(*other.root_->as_inner());
     }
 
     b_plus_tree_hash_set(b_plus_tree_hash_set&& other) noexcept
@@ -109,8 +109,8 @@ class b_plus_tree_hash_set {
     b_plus_tree_hash_set& operator=(const b_plus_tree_hash_set& other) {
         assert(this != &other);
         delete root_;
-        root_ = other.root_->is_leaf() ? new NLeafT(*other.root_->as_leaf())
-                                       : new NInnerT(*other.root_->as_inner());
+        root_ = other.root_->is_leaf() ? (NodeT*)new NLeafT(*other.root_->as_leaf())
+                                       : (NodeT*)new NInnerT(*other.root_->as_inner());
         size_ = other.size_;
         return *this;
     }
@@ -126,7 +126,6 @@ class b_plus_tree_hash_set {
 
     ~b_plus_tree_hash_set() {
         delete root_;
-        root_ = nullptr;
     }
 
     [[nodiscard]] auto find(const T& value) {

diff --git a/include/phtree/common/b_plus_tree_map.h b/include/phtree/common/b_plus_tree_map.h
@@ -100,8 +100,8 @@ class b_plus_tree_map {
     explicit b_plus_tree_map() : root_{new NLeafT(nullptr, nullptr, nullptr)}, size_{0} {};
 
     b_plus_tree_map(const b_plus_tree_map& other) : size_{other.size_} {
-        root_ = other.root_->is_leaf() ? new NLeafT(*other.root_->as_leaf())
-                                       : new NInnerT(*other.root_->as_inner());
+        root_ = other.root_->is_leaf() ? (NodeT*)new NLeafT(*other.root_->as_leaf())
+                                       : (NodeT*)new NInnerT(*other.root_->as_inner());
     }
 
     b_plus_tree_map(b_plus_tree_map&& other) noexcept : root_{other.root_}, size_{other.size_} {
@@ -112,8 +112,8 @@ class b_plus_tree_map {
     b_plus_tree_map& operator=(const b_plus_tree_map& other) {
         assert(this != &other);
         delete root_;
-        root_ = other.root_->is_leaf() ? new NLeafT(*other.root_->as_leaf())
-                                       : new NInnerT(*other.root_->as_inner());
+        root_ = other.root_->is_leaf() ? (NodeT*)new NLeafT(*other.root_->as_leaf())
+                                       : (NodeT*)new NInnerT(*other.root_->as_inner());
         size_ = other.size_;
         return *this;
     }
@@ -129,7 +129,6 @@ class b_plus_tree_map {
 
     ~b_plus_tree_map() {
         delete root_;
-        root_ = nullptr;
     }
 
     [[nodiscard]] auto find(KeyT key) noexcept {

diff --git a/include/phtree/common/b_plus_tree_multimap.h b/include/phtree/common/b_plus_tree_multimap.h
@@ -123,7 +123,6 @@ class b_plus_tree_multimap {
 
     ~b_plus_tree_multimap() {
         delete root_;
-        root_ = nullptr;
     }
 
     [[nodiscard]] auto find(const KeyT key) {

diff --git a/test/common/b_plus_tree_hash_map_test.cc b/test/common/b_plus_tree_hash_map_test.cc
@@ -390,3 +390,90 @@ TEST(PhTreeBptHashMapTest, SmokeTestWithEraseSameHash) {
     SmokeTestWithErase<DumbHash>(true);
     SmokeTestWithErase<DumbHash>(false);
 }
+
+template <typename TREE>
+void test_tree(TREE& tree) {
+    using Key = size_t;
+    using Value = size_t;
+    Key p{42};
+
+    // test various operations
+    tree.emplace(p, Value{2});
+    Value id3{3};
+    tree.emplace(p, id3);
+    ASSERT_EQ(tree.size(), 1u);
+
+    auto q_extent = tree.begin();
+    ASSERT_NE(q_extent, tree.end());
+    ++q_extent;
+    ASSERT_EQ(q_extent, tree.end());
+
+    tree.erase(p);
+    ASSERT_EQ(0u, tree.size());
+    tree.erase(p);
+    ASSERT_EQ(0u, tree.size());
+    ASSERT_TRUE(tree.empty());
+}
+
+TEST(PhTreeBptHashMapTest, TestCopyConstruct) {
+    using TestTree = b_plus_tree_hash_map<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{tree1};
+    test_tree(tree);
+    // The old tree should still work!
+    test_tree(tree1);
+}
+
+TEST(PhTreeBptHashMapTest, TestCopyAssign) {
+    using TestTree = b_plus_tree_hash_map<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{};
+    tree = tree1;
+    test_tree(tree);
+    // The old tree should still work!
+    test_tree(tree1);
+}
+
+TEST(PhTreeBptHashMapTest, TestMoveConstruct) {
+    using TestTree = b_plus_tree_hash_map<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{std::move(tree1)};
+    test_tree(tree);
+}
+
+TEST(PhTreeBptHashMapTest, TestMoveAssign) {
+    using TestTree = b_plus_tree_hash_map<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{};
+    tree = std::move(tree1);
+    test_tree(tree);
+}
+
+TEST(PhTreeBptHashMapTest, TestMovableIterators) {
+    using Key = size_t;
+    using Value = size_t;
+    using TestTree = b_plus_tree_hash_map<Key, Value>;
+    // Test edge case: only one entry in tree
+    Key p{42};
+    auto tree = TestTree();
+    tree.emplace(p, Value{1});
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.begin())>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.begin())>);
+    ASSERT_NE(tree.begin(), tree.end());
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.end())>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.end())>);
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.find(p))>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.find(p))>);
+    ASSERT_NE(tree.find(p), tree.end());
+}
diff --git a/test/common/b_plus_tree_map_test.cc b/test/common/b_plus_tree_map_test.cc
@@ -181,3 +181,90 @@ TEST(PhTreeBptMapTest, SmokeTestLowerBound) {
         }
     }
 }
+
+template <typename TREE>
+void test_tree(TREE& tree) {
+    using Key = size_t;
+    using Value = size_t;
+    Key p{42};
+
+    // test various operations
+    tree.emplace(p, Value{2});
+    Value id3{3};
+    tree.emplace(p, id3);
+    ASSERT_EQ(tree.size(), 1u);
+
+    auto q_extent = tree.begin();
+    ASSERT_NE(q_extent, tree.end());
+    ++q_extent;
+    ASSERT_EQ(q_extent, tree.end());
+
+    tree.erase(p);
+    ASSERT_EQ(0u, tree.size());
+    tree.erase(p);
+    ASSERT_EQ(0u, tree.size());
+    ASSERT_TRUE(tree.empty());
+}
+
+TEST(PhTreeBptMapTest, TestCopyConstruct) {
+    using TestTree = b_plus_tree_map<size_t, size_t, 16>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{tree1};
+    test_tree(tree);
+    // The old tree should still work!
+    test_tree(tree1);
+}
+
+TEST(PhTreeBptMapTest, TestCopyAssign) {
+    using TestTree = b_plus_tree_map<size_t, size_t, 16>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{};
+    tree = tree1;
+    test_tree(tree);
+    // The old tree should still work!
+    test_tree(tree1);
+}
+
+TEST(PhTreeBptMapTest, TestMoveConstruct) {
+    using TestTree = b_plus_tree_map<size_t, size_t, 16>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{std::move(tree1)};
+    test_tree(tree);
+}
+
+TEST(PhTreeBptMapTest, TestMoveAssign) {
+    using TestTree = b_plus_tree_map<size_t, size_t, 16>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{};
+    tree = std::move(tree1);
+    test_tree(tree);
+}
+
+TEST(PhTreeBptMapTest, TestMovableIterators) {
+    using Key = size_t;
+    using Value = size_t;
+    using TestTree = b_plus_tree_map<Key, Value, 16>;
+    // Test edge case: only one entry in tree
+    Key p{42};
+    auto tree = TestTree();
+    tree.emplace(p, Value{1});
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.begin())>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.begin())>);
+    ASSERT_NE(tree.begin(), tree.end());
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.end())>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.end())>);
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.find(p))>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.find(p))>);
+    ASSERT_NE(tree.find(p), tree.end());
+}
diff --git a/test/common/b_plus_tree_multimap_test.cc b/test/common/b_plus_tree_multimap_test.cc
@@ -446,3 +446,94 @@ TEST(PhTreeBptMulitmapTest, SmokeTestUpdateByIterator) {
         }
     }
 }
+
+template <typename TREE>
+void test_tree(TREE& tree) {
+    using Key = size_t;
+    using Value = size_t;
+    Key p{42};
+
+    // test various operations
+    tree.emplace(p, Value{2});
+    Value id3{3};
+    tree.emplace(p, id3);
+    ASSERT_EQ(tree.size(), 3u);
+
+    auto q_extent = tree.begin();
+    ASSERT_NE(q_extent, tree.end());
+    ++q_extent;
+    ASSERT_NE(q_extent, tree.end());
+    ++q_extent;
+    ASSERT_NE(q_extent, tree.end());
+    ++q_extent;
+    ASSERT_EQ(q_extent, tree.end());
+
+    ASSERT_EQ(3u, tree.erase(p));
+    ASSERT_EQ(0u, tree.size());
+    ASSERT_EQ(0u, tree.erase(p));
+    ASSERT_EQ(0u, tree.size());
+    ASSERT_TRUE(tree.empty());
+}
+
+TEST(PhTreeBptMulitmapTest, TestCopyConstruct) {
+    using TestTree = b_plus_tree_multimap<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{tree1};
+    test_tree(tree);
+    // The old tree should still work!
+    test_tree(tree1);
+}
+
+TEST(PhTreeBptMulitmapTest, TestCopyAssign) {
+    using TestTree = b_plus_tree_multimap<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{};
+    tree = tree1;
+    test_tree(tree);
+    // The old tree should still work!
+    test_tree(tree1);
+}
+
+TEST(PhTreeBptMulitmapTest, TestMoveConstruct) {
+    using TestTree = b_plus_tree_multimap<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{std::move(tree1)};
+    test_tree(tree);
+}
+
+TEST(PhTreeBptMulitmapTest, TestMoveAssign) {
+    using TestTree = b_plus_tree_multimap<size_t, size_t>;
+    TestTree tree1;
+    tree1.emplace(42, 1);
+
+    TestTree tree{};
+    tree = std::move(tree1);
+    test_tree(tree);
+}
+
+TEST(PhTreeBptMulitmapTest, TestMovableIterators) {
+    using Key = size_t;
+    using Value = size_t;
+    using TestTree = b_plus_tree_multimap<Key, Value>;
+    // Test edge case: only one entry in tree
+    Key p{42};
+    auto tree = TestTree();
+    tree.emplace(p, Value{1});
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.begin())>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.begin())>);
+    ASSERT_NE(tree.begin(), tree.end());
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.end())>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.end())>);
+
+    ASSERT_TRUE(std::is_move_constructible_v<decltype(tree.find(p))>);
+    ASSERT_TRUE(std::is_move_assignable_v<decltype(tree.find(p))>);
+    ASSERT_NE(tree.find(p), tree.end());
+}
diff --git a/test/phtree_multimap_d_test.cc b/test/phtree_multimap_d_test.cc
@@ -1248,7 +1248,6 @@ TEST(PhTreeMMDTest, TestMoveConstruct) {
 
     TestTree<3, Id> tree{std::move(tree1)};
     test_tree(tree);
-    tree.~PhTreeMultiMap();
 }
 
 TEST(PhTreeMMDTest, TestMoveAssign) {
@@ -1260,7 +1259,6 @@ TEST(PhTreeMMDTest, TestMoveAssign) {
     TestTree<3, Id> tree{};
     tree = std::move(tree1);
     test_tree(tree);
-    tree.~PhTreeMultiMap();
 }
 
 TEST(PhTreeMMDTest, TestMovableIterators) {

diff --git a/test/phtree_test.cc b/test/phtree_test.cc
@@ -1285,7 +1285,6 @@ TEST(PhTreeTest, TestMoveConstruct) {
 
     TestTree<3, Id> tree{std::move(tree1)};
     test_tree(tree);
-    tree.~PhTree();
 }
 
 TEST(PhTreeTest, TestMoveAssign) {
@@ -1297,7 +1296,6 @@ TEST(PhTreeTest, TestMoveAssign) {
     TestTree<3, Id> tree{};
     tree = std::move(tree1);
     test_tree(tree);
-    tree.~PhTree();
 }
 
 size_t count_pre{0};
@@ -1351,7 +1349,6 @@ TEST(PhTreeTest, TestMoveAssignCustomConverter) {
     test_tree(tree);
     ASSERT_GE(tree.converter().count_pre_local, 2);
     ASSERT_EQ(tree.converter().count_pre_local, count_pre);
-    tree.~PhTree();
 }
 
 TEST(PhTreeTest, TestMovableIterators) {