diff --git a/c++/cavl.hpp b/c++/cavl.hpp index 2516d71..5389915 100644 --- a/c++/cavl.hpp +++ b/c++/cavl.hpp @@ -86,6 +86,11 @@ class Node // NOSONAR cpp:S1448 cpp:S4963 Node(Node&& other) noexcept { moveFrom(other); } auto operator=(Node&& other) noexcept -> Node& { + if (isLinked()) + { + remove(); + } + moveFrom(other); return *this; } @@ -102,6 +107,14 @@ class Node // NOSONAR cpp:S1448 cpp:S4963 auto getChildNode(const bool right) noexcept -> Derived* { return down(lr[right]); } auto getChildNode(const bool right) const noexcept -> const Derived* { return down(lr[right]); } auto getBalanceFactor() const noexcept { return bf; } + auto getNextInOrderNode(const bool reverse = false) noexcept -> Derived* + { + return getNextInOrderNodeImpl(this, reverse); + } + auto getNextInOrderNode(const bool reverse = false) const noexcept -> const Derived* + { + return getNextInOrderNodeImpl(this, reverse); + } /// Find a node for which the predicate returns zero, or nullptr if there is no such node or the tree is empty. /// The predicate is invoked with a single argument which is a constant reference to Derived. @@ -128,25 +141,23 @@ class Node // NOSONAR cpp:S1448 cpp:S4963 template static auto search(Node& origin, const Pre& predicate, const Fac& factory) -> std::tuple; - /// Remove the specified node from its tree. The root node (inside the origin) may be replaced in the process. - /// The function has no effect if the node pointer is nullptr. - /// If the node is not in the tree, the behavior is undefined; it may create cycles in the tree which is deadly. - /// It is safe to pass the result of search() directly as the second argument: - /// Node::remove(root, Node::search(root, search_predicate)); + /// Remove the specified node from its tree. /// + /// The root node (inside the origin) may be replaced in the process. /// No Sonar cpp:S6936 b/c the `remove` method name isolated inside `Node` type (doesn't conflict with C). - static void remove(Node& origin, const Node* const node) noexcept; // NOSONAR cpp:S6936 + void remove() const noexcept // NOSONAR cpp:S6936 + { + removeImpl(this); + } - /// This is like the const overload of remove() except that the node pointers are invalidated afterward for safety. + /// This is like the const overload of `remove()` + /// except that the node pointers are invalidated afterward for safety. /// /// No Sonar cpp:S6936 b/c the `remove` method name isolated inside `Node` type (doesn't conflict with C). - static void remove(Node& origin, Node* const node) noexcept // NOSONAR cpp:S6936 + void remove() noexcept // NOSONAR cpp:S6936 { - remove(origin, static_cast(node)); - if (nullptr != node) - { - node->unlink(); - } + removeImpl(this); + unlink(); } /// These methods provide very fast retrieval of min/max values, either const or mutable. @@ -264,6 +275,8 @@ class Node // NOSONAR cpp:S1448 cpp:S4963 template static void traversePostOrderImpl(DerivedT* const root, const Vis& visitor, const bool reverse); + static void removeImpl(const Node* const node) noexcept; + template static auto searchImpl(NodeT* const root, const Pre& predicate) noexcept -> DerivedT* { @@ -283,6 +296,29 @@ class Node // NOSONAR cpp:S1448 cpp:S4963 return nullptr; } + template + static auto getNextInOrderNodeImpl(NodeT* const node, const bool reverse) noexcept -> DerivedT* + { + if (nullptr != node->lr[!reverse]) + { + auto* next_down = node->lr[!reverse]; + while (nullptr != next_down->lr[reverse]) + { + next_down = next_down->lr[reverse]; + } + return down(next_down); + } + + auto* next_up_child = node; + NodeT* next_up = node->getParentNode(); + while ((nullptr != next_up) && (next_up_child == next_up->lr[!reverse])) + { + next_up_child = next_up; + next_up = next_up->getParentNode(); + } + return down(next_up); + } + void unlink() noexcept { up = nullptr; @@ -382,103 +418,82 @@ auto Node::search(Node& origin, const Pre& predicate, const Fac& factor return std::make_tuple(down(out), false); } -// No Sonar cpp:S6936 b/c the `remove` method name isolated inside `Node` type (doesn't conflict with C). -// No Sonar cpp:S3776 cpp:S134 cpp:S5311 b/c this is the main removal tool - maintainability is not a concern here. template -void Node::remove(Node& origin, const Node* const node) noexcept // NOSONAR cpp:S6936 cpp:S3776 +void Node::removeImpl(const Node* const node) noexcept { - CAVL_ASSERT(!origin.isLinked()); - CAVL_ASSERT(node != &origin); // The origin node is not part of the tree, so it cannot be removed. - - if (node != nullptr) - { - Node*& root = origin.lr[0]; - CAVL_ASSERT(root != nullptr); // Otherwise, the node would have to be nullptr. - CAVL_ASSERT(node->isLinked()); - Node* p = nullptr; // The lowest parent node that suffered a shortening of its subtree. - bool r = false; // Which side of the above was shortened. - // The first step is to update the topology and remember the node where to start the retracing from later. - // Balancing is not performed yet, so we may end up with an unbalanced tree. - if ((node->lr[0] != nullptr) && (node->lr[1] != nullptr)) + CAVL_ASSERT(node != nullptr); + CAVL_ASSERT(node->isLinked()); + + Node* p = nullptr; // The lowest parent node that suffered a shortening of its subtree. + bool r = false; // Which side of the above was shortened. + // The first step is to update the topology and remember the node where to start the retracing from later. + // Balancing is not performed yet, so we may end up with an unbalanced tree. + if ((node->lr[0] != nullptr) && (node->lr[1] != nullptr)) + { + Node* const re = min(node->lr[1]); + CAVL_ASSERT((re != nullptr) && (nullptr == re->lr[0]) && (re->up != nullptr)); + re->bf = node->bf; + re->lr[0] = node->lr[0]; + re->lr[0]->up = re; + if (re->up != node) { - Node* const re = min(node->lr[1]); - CAVL_ASSERT((re != nullptr) && (nullptr == re->lr[0]) && (re->up != nullptr)); - re->bf = node->bf; - re->lr[0] = node->lr[0]; - re->lr[0]->up = re; - if (re->up != node) - { - p = re->getParentNode(); // Retracing starts with the ex-parent of our replacement node. - CAVL_ASSERT(p->lr[0] == re); - p->lr[0] = re->lr[1]; // Reducing the height of the left subtree here. - if (p->lr[0] != nullptr) // NOSONAR cpp:S134 - { - p->lr[0]->up = p; - } - re->lr[1] = node->lr[1]; - re->lr[1]->up = re; - r = false; - } - else // In this case, we are reducing the height of the right subtree, so r=1. - { - p = re; // Retracing starts with the replacement node itself as we are deleting its parent. - r = true; // The right child of the replacement node remains the same, so we don't bother relinking it. - } - re->up = node->up; - if (!re->isRoot()) + p = re->getParentNode(); // Retracing starts with the ex-parent of our replacement node. + CAVL_ASSERT(p->lr[0] == re); + p->lr[0] = re->lr[1]; // Reducing the height of the left subtree here. + if (p->lr[0] != nullptr) { - re->up->lr[re->up->lr[1] == node] = re; // Replace link in the parent of node. - } - else - { - root = re; + p->lr[0]->up = p; } + re->lr[1] = node->lr[1]; + re->lr[1]->up = re; + r = false; } - else // Either or both of the children are nullptr. + else // In this case, we are reducing the height of the right subtree, so r=1. { - p = node->up; - const bool rr = node->lr[1] != nullptr; - if (node->lr[rr] != nullptr) - { - node->lr[rr]->up = p; - } - if (!node->isRoot()) - { - r = p->lr[1] == node; - p->lr[r] = node->lr[rr]; - if (p->lr[r] != nullptr) // NOSONAR cpp:S134 - { - p->lr[r]->up = p; - } - } - else - { - root = node->lr[rr]; - } + p = re; // Retracing starts with the replacement node itself as we are deleting its parent. + r = true; // The right child of the replacement node remains the same, so we don't bother relinking it. } - // Now that the topology is updated, perform the retracing to restore balance. We climb up adjusting the - // balance factors until we reach the root or a parent whose balance factor becomes plus/minus one, which - // means that that parent was able to absorb the balance delta; in other words, the height of the outer - // subtree is unchanged, so upper balance factors shall be kept unchanged. - if (p != &origin) + re->up = node->up; + re->up->lr[re->up->lr[1] == node] = re; // Replace link in the parent of node. + } + else // Either or both of the children are nullptr. + { + p = node->up; + const bool rr = node->lr[1] != nullptr; + if (node->lr[rr] != nullptr) { - Node* c = nullptr; - for (;;) // NOSONAR cpp:S5311 + node->lr[rr]->up = p; + } + + r = p->lr[1] == node; + p->lr[r] = node->lr[rr]; + if (p->lr[r] != nullptr) + { + p->lr[r]->up = p; + } + } + // Now that the topology is updated, perform the retracing to restore balance. We climb up adjusting the + // balance factors until we reach the root or a parent whose balance factor becomes plus/minus one, which + // means that that parent was able to absorb the balance delta; in other words, the height of the outer + // subtree is unchanged, so upper balance factors shall be kept unchanged. + if (p->isLinked()) + { + for (;;) // NOSONAR cpp:S5311 + { + Node* const c = p->adjustBalance(!r); + CAVL_ASSERT(nullptr != c); + p = c->getParentNode(); + if ((c->bf != 0) || (nullptr == p)) { - c = p->adjustBalance(!r); - p = c->getParentNode(); - if ((c->bf != 0) || (nullptr == p)) // NOSONAR cpp:S134 + // Reached the root or the height difference is absorbed by `c`. + CAVL_ASSERT(nullptr != c->up); + if ((nullptr == p) && (nullptr != c->up)) // NOSONAR cpp:S134 { - // Reached the root or the height difference is absorbed by `c`. - break; + c->up->lr[0] = c; } - r = p->lr[1] == c; - } - if (nullptr == p) - { - CAVL_ASSERT(c != nullptr); - root = c; + break; } + r = p->lr[1] == c; } } } @@ -541,6 +556,7 @@ auto Node::adjustBalance(const bool increment) noexcept -> Node* { bf = new_bf; // Balancing not needed, just update the balance factor and call it a day. } + CAVL_ASSERT(nullptr != out); return out; } @@ -744,11 +760,14 @@ class Tree final // NOSONAR cpp:S3624 /// Trees can be easily moved in constant time. This does not actually affect the tree itself, only this object. Tree(Tree&& other) noexcept : origin_node_{std::move(other.origin_node_)} { - CAVL_ASSERT(!traversal_in_progress_); // Cannot modify the tree while it is being traversed. + CAVL_ASSERT(!traversal_in_progress_); // Cannot modify the tree while it is being traversed. + CAVL_ASSERT(!other.traversal_in_progress_); // Cannot modify the tree while it is being traversed. } auto operator=(Tree&& other) noexcept -> Tree& { - CAVL_ASSERT(!traversal_in_progress_); // Cannot modify the tree while it is being traversed. + CAVL_ASSERT(!traversal_in_progress_); // Cannot modify the tree while it is being traversed. + CAVL_ASSERT(!other.traversal_in_progress_); // Cannot modify the tree while it is being traversed. + origin_node_ = std::move(other.origin_node_); return *this; } @@ -771,13 +790,26 @@ class Tree final // NOSONAR cpp:S3624 return NodeType::template search(origin_node_, predicate, factory); } + /// The function has no effect if the node pointer is `nullptr`, or node is not in the tree (aka unlinked). + /// It is safe to pass the result of search() directly as the `node` argument: + /// `tree.remove(Node::search(root, search_predicate));` + /// but it could be done also by direct node removal: + /// ``` + /// if (auto* const node = Node::search(root, search_predicate) { + /// node->remove(); + /// } + /// ``` + /// /// Wraps NodeType<>::remove(). /// /// No Sonar cpp:S6936 b/c the `remove` method name isolated inside `Tree` type (doesn't conflict with C). void remove(NodeType* const node) noexcept // NOSONAR cpp:S6936 { CAVL_ASSERT(!traversal_in_progress_); // Cannot modify the tree while it is being traversed. - NodeType::remove(origin_node_, node); + if ((node != nullptr) && node->isLinked()) + { + node->remove(); + } } /// Wraps NodeType<>::min/max(). diff --git a/c++/test.cpp b/c++/test.cpp index 9d30ae2..23f98bc 100644 --- a/c++/test.cpp +++ b/c++/test.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #if __cplusplus >= 201703L @@ -47,12 +48,14 @@ constexpr auto Zzzzz = nullptr; class My : public cavl::Node { public: + My() = default; explicit My(const std::uint16_t v) : value(v) {} using Self = cavl::Node; using Self::isLinked; using Self::isRoot; using Self::getChildNode; using Self::getParentNode; + using Self::getNextInOrderNode; using Self::getBalanceFactor; using Self::search; using Self::remove; @@ -157,8 +160,7 @@ template void checkPostOrdering(const N* const root, const std::vector& expected, const bool reverse = false) { std::vector order; - T::traversePostOrder( - root, [&](const N& nd) { order.push_back(nd.getValue()); }, reverse); + T::traversePostOrder(root, [&](const N& nd) { order.push_back(nd.getValue()); }, reverse); TEST_ASSERT_EQUAL(expected.size(), order.size()); if (!order.empty()) { @@ -273,7 +275,7 @@ void testManual(const std::function& factory, const std::funct TEST_ASSERT_NULL(tr.search(pred)); TEST_ASSERT_NULL(static_cast(tr).search(pred)); TEST_ASSERT_FALSE(t[i]->isLinked()); - auto result = tr.search(pred, [&]() { return t[i]; }); + auto result = tr.search(pred, [&] { return t[i]; }); TEST_ASSERT_TRUE(t[i]->isLinked()); TEST_ASSERT_EQUAL(t[i], std::get<0>(result)); TEST_ASSERT_FALSE(std::get<1>(result)); @@ -324,6 +326,13 @@ void testManual(const std::function& factory, const std::funct TEST_ASSERT_NOT_NULL(tr[i - 1]); TEST_ASSERT_EQUAL_INT64(i, tr[i - 1]->getValue()); TEST_ASSERT_EQUAL_INT64(i, static_cast(tr)[i - 1]->getValue()); + + // Check the in-order successor. + TEST_ASSERT_EQUAL(i < 31 ? t.at(i + 1) : nullptr, + static_cast(tr)[i - 1]->getNextInOrderNode()); + // Check the reverse in-order predecessor. + const auto ri = 32 - i; + TEST_ASSERT_EQUAL(ri > 1 ? t.at(ri - 1) : nullptr, tr[ri - 1]->getNextInOrderNode(true)); // reverse } checkPostOrdering(tr, {1, 3, 2, 5, 7, 6, 4, 9, 11, 10, 13, 15, 14, 12, 8, 17, 19, 18, 21, 23, 22, 20, 25, 27, 26, 29, 31, 30, 28, 24, 16}); @@ -869,7 +878,7 @@ void testRandomized() std::uint64_t cnt_addition = 0; std::uint64_t cnt_removal = 0; - const auto validate = [&]() { + const auto validate = [&] { TEST_ASSERT_EQUAL(size, std::accumulate(mask.begin(), mask.end(), 0U, [](const std::size_t a, const std::size_t b) { return a + b; @@ -957,6 +966,18 @@ void testRandomized() void testManualMy() { + static_assert(!std::is_copy_assignable::value, "Should not be copy assignable."); + static_assert(!std::is_copy_constructible::value, "Should not be copy constructible."); + static_assert(std::is_move_assignable::value, "Should be move assignable."); + static_assert(std::is_move_constructible::value, "Should be move constructible."); + static_assert(std::is_default_constructible::value, "Should be default constructible."); + + static_assert(!std::is_copy_assignable::value, "Should not be copy assignable."); + static_assert(!std::is_copy_constructible::value, "Should not be copy constructible."); + static_assert(std::is_move_assignable::value, "Should be move assignable."); + static_assert(std::is_move_constructible::value, "Should be move constructible."); + static_assert(std::is_default_constructible::value, "Should be default constructible."); + testManual( [](const std::uint16_t x) { return new My(x); // NOLINT @@ -979,6 +1000,7 @@ class V : public cavl::Node using Self::isRoot; using Self::getChildNode; using Self::getParentNode; + using Self::getNextInOrderNode; using Self::getBalanceFactor; using Self::search; using Self::remove; @@ -1001,7 +1023,6 @@ class V : public cavl::Node private: using E = struct {}; - UNUSED E root_ptr; UNUSED E up; UNUSED E lr; UNUSED E bf; @@ -1063,6 +1084,18 @@ auto makeV(const std::uint8_t val) -> V* void testManualV() { + static_assert(!std::is_copy_assignable::value, "Should not be copy assignable."); + static_assert(!std::is_copy_constructible::value, "Should not be copy constructible."); + static_assert(std::is_move_assignable::value, "Should be move assignable."); + static_assert(!std::is_move_constructible::value, "Should be move constructible."); + static_assert(!std::is_default_constructible::value, "Should be default constructible."); + + static_assert(!std::is_copy_assignable::value, "Should not be copy assignable."); + static_assert(!std::is_copy_constructible::value, "Should not be copy constructible."); + static_assert(std::is_move_assignable::value, "Should be move assignable."); + static_assert(std::is_move_constructible::value, "Should be move constructible."); + static_assert(std::is_default_constructible::value, "Should be default constructible."); + testManual(&makeV<>, [](V* const old_node) { // auto* const new_node = old_node->clone(); delete old_node; // NOLINT(*-owning-memory)