From e9543ab8ec9daff327abda6ba11211c66148391f Mon Sep 17 00:00:00 2001 From: Jeremiah Morgan Date: Fri, 24 Jan 2025 18:13:43 +0000 Subject: [PATCH 1/2] fix paring heap UB --- libopenage/datastructure/pairing_heap.h | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/libopenage/datastructure/pairing_heap.h b/libopenage/datastructure/pairing_heap.h index dd348d8c55..8cf1731713 100644 --- a/libopenage/datastructure/pairing_heap.h +++ b/libopenage/datastructure/pairing_heap.h @@ -1,4 +1,4 @@ -// Copyright 2014-2024 the openage authors. See copying.md for legal info. +// Copyright 2014-2025 the openage authors. See copying.md for legal info. #pragma once @@ -404,7 +404,7 @@ class PairingHeap final { * erase all elements on the heap. */ void clear() { - auto delete_node = [](element_t node) { delete node; }; + auto delete_node = [](element_t& node) { delete node; node = nullptr; }; this->iter_all(delete_node); this->root_node = nullptr; this->node_count = 0; @@ -586,7 +586,7 @@ class PairingHeap final { * @param func Function to apply to each node. */ template - void iter_all(const std::function &func) const { + void iter_all(const std::function &func) { this->walk_tree(this->root_node, func); } @@ -599,21 +599,18 @@ class PairingHeap final { * @param func Function to apply to each node. */ template - void walk_tree(const element_t &start, - const std::function &func) const { + void walk_tree(element_t &start, + const std::function &func) { if constexpr (not reverse) { func(start); } if (start) { auto node = start->first_child; - while (true) { - if (not node) { - break; - } - + while (node) { this->walk_tree(node, func); - node = node->next_sibling; + if (node) + node = node->next_sibling; } if constexpr (reverse) { func(start); From aef6cf01ecf64bb1c9e3b10f4ed9b1621dcf0fd0 Mon Sep 17 00:00:00 2001 From: Jeremiah Morgan Date: Sun, 26 Jan 2025 16:05:47 +0000 Subject: [PATCH 2/2] add iterator to paring heap --- libopenage/datastructure/pairing_heap.h | 206 +++++++++++++++++++++++- libopenage/datastructure/tests.cpp | 10 +- 2 files changed, 207 insertions(+), 9 deletions(-) diff --git a/libopenage/datastructure/pairing_heap.h b/libopenage/datastructure/pairing_heap.h index 8cf1731713..df8d1ad66d 100644 --- a/libopenage/datastructure/pairing_heap.h +++ b/libopenage/datastructure/pairing_heap.h @@ -17,6 +17,7 @@ */ #include +#include #include #include #include @@ -36,6 +37,9 @@ template class PairingHeap; +template +class PairingHeapIterator; + template > class PairingHeapNode { @@ -43,6 +47,7 @@ class PairingHeapNode { using this_type = PairingHeapNode; friend PairingHeap; + friend PairingHeapIterator; T data; compare cmp; @@ -186,6 +191,166 @@ class PairingHeapNode { }; +/** + * @brief Iterator class for PairingHeap. + * + * This class provides a bidirectional iterator for the PairingHeap data structure. + * It allows traversal of the heap in both forward and backward directions. + * It is depth-first traversal. + * + * @tparam T The type of elements stored in the heap. + * @tparam compare The comparison functor used to order the elements. + */ +template > +class PairingHeapIterator { +public: + using iterator_category = std::bidirectional_iterator_tag; + using value_type = T; + using difference_type = std::ptrdiff_t; + using pointer = T *; + using reference = T &; + + /** + * @brief Constructs an iterator starting at the given node. + * + * @param node The starting node for the iterator. + */ + PairingHeapIterator(PairingHeapNode *node) : + current(node) {} + + /** + * @brief Dereference operator. + * + * @return A reference to the data stored in the current node. + */ + reference operator*() const { + return current->data; + } + + /** + * @brief Member access operator. + * + * @return A pointer to the data stored in the current node. + */ + pointer operator->() const { + return &(current->data); + } + + + /** + * @brief Get current node. + * + * @return The current node. + */ + PairingHeapNode *node() { + return current; + } + + + /** + * @brief Pre-increment operator. + * + * Moves the iterator to the next node in the heap. + * + * @return A reference to the incremented iterator. + */ + PairingHeapIterator &operator++() { + if (current->first_child) { + current = current->first_child; + } + else if (current->next_sibling) { + current = current->next_sibling; + } + else { + while (current->parent && !current->parent->next_sibling) { + current = current->parent; + } + if (current->parent) { + current = current->parent->next_sibling; + } + else { + current = nullptr; + } + } + return *this; + } + + /** + * @brief Post-increment operator. + * + * Moves the iterator to the next node in the heap. + * + * @return A copy of the iterator before incrementing. + */ + PairingHeapIterator operator++(int) { + PairingHeapIterator tmp = *this; + ++(*this); + return tmp; + } + + /** + * @brief Pre-decrement operator. + * + * Moves the iterator to the previous node in the heap. + * + * @return A reference to the decremented iterator. + */ + PairingHeapIterator &operator--() { + if (current->prev_sibling) { + current = current->prev_sibling; + while (current->first_child) { + current = current->first_child; + while (current->next_sibling) { + current = current->next_sibling; + } + } + } + else if (current->parent) { + current = current->parent; + } + return *this; + } + + /** + * @brief Post-decrement operator. + * + * Moves the iterator to the previous node in the heap. + * + * @return A copy of the iterator before decrementing. + */ + PairingHeapIterator operator--(int) { + PairingHeapIterator tmp = *this; + --(*this); + return tmp; + } + + /** + * @brief Equality comparison operator. + * + * @param a The first iterator to compare. + * @param b The second iterator to compare. + * @return True if both iterators point to the same node, false otherwise. + */ + friend bool operator==(const PairingHeapIterator &a, const PairingHeapIterator &b) { + return a.current == b.current; + } + + /** + * @brief Inequality comparison operator. + * + * @param a The first iterator to compare. + * @param b The second iterator to compare. + * @return True if the iterators point to different nodes, false otherwise. + */ + friend bool operator!=(const PairingHeapIterator &a, const PairingHeapIterator &b) { + return a.current != b.current; + } + +private: + PairingHeapNode *current; ///< Pointer to the current node in the heap. +}; + + /** * (Quite) efficient heap implementation. */ @@ -196,6 +361,7 @@ class PairingHeap final { public: using element_t = heapnode_t *; using this_type = PairingHeap; + using iterator = PairingHeapIterator; /** * create a empty heap. @@ -404,11 +570,24 @@ class PairingHeap final { * erase all elements on the heap. */ void clear() { - auto delete_node = [](element_t& node) { delete node; node = nullptr; }; - this->iter_all(delete_node); + std::vector to_delete; + to_delete.reserve(this->size()); + + // collect all node pointers to delete + for (iterator it = this->begin(); it != this->end(); it++) { + to_delete.push_back(it.node()); + } + + // delete all nodes + for (element_t node : to_delete) { + delete node; + } + + // reset heap state to empty this->root_node = nullptr; this->node_count = 0; #if OPENAGE_PAIRINGHEAP_DEBUG + // clear the node set for debugging this->nodes.clear(); #endif } @@ -586,10 +765,18 @@ class PairingHeap final { * @param func Function to apply to each node. */ template - void iter_all(const std::function &func) { + void iter_all(const std::function &func) const { this->walk_tree(this->root_node, func); } + iterator begin() const { + return iterator(this->root_node); + } + + iterator end() const { + return iterator(nullptr); + } + private: /** * Apply the given function to all nodes in the tree. @@ -599,18 +786,21 @@ class PairingHeap final { * @param func Function to apply to each node. */ template - void walk_tree(element_t &start, - const std::function &func) { + void walk_tree(const element_t &start, + const std::function &func) const { if constexpr (not reverse) { func(start); } if (start) { auto node = start->first_child; - while (node) { + while (true) { + if (not node) { + break; + } + this->walk_tree(node, func); - if (node) - node = node->next_sibling; + node = node->next_sibling; } if constexpr (reverse) { func(start); diff --git a/libopenage/datastructure/tests.cpp b/libopenage/datastructure/tests.cpp index c375f78c17..40aa516951 100644 --- a/libopenage/datastructure/tests.cpp +++ b/libopenage/datastructure/tests.cpp @@ -1,4 +1,4 @@ -// Copyright 2014-2024 the openage authors. See copying.md for legal info. +// Copyright 2014-2025 the openage authors. See copying.md for legal info. #include "tests.h" @@ -135,6 +135,14 @@ void pairing_heap_3() { heap.push(heap_elem{3}); heap.push(heap_elem{4}); heap.push(heap_elem{5}); + + size_t i = 0; + std::array expected{0, 5, 4, 3, 2, 1}; + for (auto &elem : heap) { + TESTEQUALS(elem.data, expected.at(i)); + i++; + } + heap.pop(); // trigger pairing heap.clear();