diff --git a/CHANGELOG.md b/CHANGELOG.md index cc3de5b6..ed27a83a 100644 --- 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 index d4335367..092f0885 100644 --- 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 index 8d8edbdb..d7e7e49d 100644 --- 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 index a929ea9a..0e9c47c6 100644 --- 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 index ed168f0c..98c04a28 100644 --- 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(true); SmokeTestWithErase(false); } + +template +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; + 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; + 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; + TestTree tree1; + tree1.emplace(42, 1); + + TestTree tree{std::move(tree1)}; + test_tree(tree); +} + +TEST(PhTreeBptHashMapTest, TestMoveAssign) { + using TestTree = b_plus_tree_hash_map; + 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; + // 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); + ASSERT_TRUE(std::is_move_assignable_v); + ASSERT_NE(tree.begin(), tree.end()); + + ASSERT_TRUE(std::is_move_constructible_v); + ASSERT_TRUE(std::is_move_assignable_v); + + ASSERT_TRUE(std::is_move_constructible_v); + ASSERT_TRUE(std::is_move_assignable_v); + 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 index 32ea8c8c..8ae8eba9 100644 --- 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 +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; + 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; + 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; + TestTree tree1; + tree1.emplace(42, 1); + + TestTree tree{std::move(tree1)}; + test_tree(tree); +} + +TEST(PhTreeBptMapTest, TestMoveAssign) { + using TestTree = b_plus_tree_map; + 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; + // 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); + ASSERT_TRUE(std::is_move_assignable_v); + ASSERT_NE(tree.begin(), tree.end()); + + ASSERT_TRUE(std::is_move_constructible_v); + ASSERT_TRUE(std::is_move_assignable_v); + + ASSERT_TRUE(std::is_move_constructible_v); + ASSERT_TRUE(std::is_move_assignable_v); + 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 index ca44661c..2725f4dc 100644 --- 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 +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; + 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; + 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; + TestTree tree1; + tree1.emplace(42, 1); + + TestTree tree{std::move(tree1)}; + test_tree(tree); +} + +TEST(PhTreeBptMulitmapTest, TestMoveAssign) { + using TestTree = b_plus_tree_multimap; + 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; + // 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); + ASSERT_TRUE(std::is_move_assignable_v); + ASSERT_NE(tree.begin(), tree.end()); + + ASSERT_TRUE(std::is_move_constructible_v); + ASSERT_TRUE(std::is_move_assignable_v); + + ASSERT_TRUE(std::is_move_constructible_v); + ASSERT_TRUE(std::is_move_assignable_v); + ASSERT_NE(tree.find(p), tree.end()); +} diff --git a/test/phtree_multimap_d_test.cc b/test/phtree_multimap_d_test.cc index 8453111f..bc31a504 100644 --- 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 index 07c706dc..f471a52a 100644 --- 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) {