From e7f6a5f4f4b35943ce6c1d2585eb3e8452a07eac Mon Sep 17 00:00:00 2001 From: Gareth Andrew Lloyd Date: Tue, 23 Jan 2024 15:31:28 +0000 Subject: [PATCH] Fix SkipList iterators (#1635) Fix SkipList iterators and find methods to be as expected by normal C++ iterator usage --- src/utils/skip_list.hpp | 146 +++++++++++++++++++++-------- tests/e2e/interactive_mg_runner.py | 1 + tests/unit/skip_list.cpp | 16 +++- 3 files changed, 124 insertions(+), 39 deletions(-) mode change 100644 => 100755 tests/e2e/interactive_mg_runner.py diff --git a/src/utils/skip_list.hpp b/src/utils/skip_list.hpp index de4892375..a6f3d0cfd 100644 --- a/src/utils/skip_list.hpp +++ b/src/utils/skip_list.hpp @@ -1,4 +1,4 @@ -// Copyright 2023 Memgraph Ltd. +// Copyright 2024 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -591,13 +591,20 @@ class SkipList final : detail::SkipListNode_base { Iterator(TNode *node) : node_(node) {} public: - TObj &operator*() const { return node_->obj; } + using value_type = TObj; + using difference_type = std::ptrdiff_t; - TObj *operator->() const { return &node_->obj; } + Iterator() = default; + Iterator(Iterator const &) = default; + Iterator(Iterator &&) = default; + Iterator &operator=(Iterator const &) = default; + Iterator &operator=(Iterator &&) = default; - bool operator==(const Iterator &other) const { return node_ == other.node_; } + value_type &operator*() const { return node_->obj; } - bool operator!=(const Iterator &other) const { return node_ != other.node_; } + value_type *operator->() const { return &node_->obj; } + + friend bool operator==(Iterator const &lhs, Iterator const &rhs) { return lhs.node_ == rhs.node_; } Iterator &operator++() { while (true) { @@ -610,8 +617,14 @@ class SkipList final : detail::SkipListNode_base { } } + Iterator operator++(int) { + Iterator old = *this; + ++(*this); + return old; + } + private: - TNode *node_; + TNode *node_{nullptr}; }; class ConstIterator final { @@ -621,15 +634,22 @@ class SkipList final : detail::SkipListNode_base { ConstIterator(TNode *node) : node_(node) {} public: + using value_type = TObj const; + using difference_type = std::ptrdiff_t; + + ConstIterator() = default; + ConstIterator(ConstIterator const &) = default; + ConstIterator(ConstIterator &&) = default; + ConstIterator &operator=(ConstIterator const &) = default; + ConstIterator &operator=(ConstIterator &&) = default; + ConstIterator(const Iterator &it) : node_(it.node_) {} - const TObj &operator*() const { return node_->obj; } + value_type &operator*() const { return node_->obj; } - const TObj *operator->() const { return &node_->obj; } + value_type *operator->() const { return &node_->obj; } - bool operator==(const ConstIterator &other) const { return node_ == other.node_; } - - bool operator!=(const ConstIterator &other) const { return node_ != other.node_; } + friend bool operator==(ConstIterator const &lhs, ConstIterator const &rhs) { return lhs.node_ == rhs.node_; } ConstIterator &operator++() { while (true) { @@ -642,6 +662,12 @@ class SkipList final : detail::SkipListNode_base { } } + ConstIterator operator++(int) { + ConstIterator old = *this; + ++(*this); + return old; + } + private: TNode *node_; }; @@ -653,6 +679,10 @@ class SkipList final : detail::SkipListNode_base { explicit Accessor(SkipList *skiplist) : skiplist_(skiplist), id_(skiplist->gc_.AllocateId()) {} public: + using value_type = TObj; + using iterator = Iterator; + using const_iterator = ConstIterator; + ~Accessor() { if (skiplist_ != nullptr) skiplist_->gc_.ReleaseId(id_); } @@ -695,7 +725,7 @@ class SkipList final : detail::SkipListNode_base { /// @return bool indicating whether the item exists template bool contains(const TKey &key) const { - return skiplist_->template contains(key); + return skiplist_->contains(key); } /// Finds the key in the list and returns an iterator to the item. @@ -703,8 +733,17 @@ class SkipList final : detail::SkipListNode_base { /// @return Iterator to the item in the list, will be equal to `end()` when /// the key isn't found template - Iterator find(const TKey &key) const { - return skiplist_->template find(key); + Iterator find(const TKey &key) { + return skiplist_->find(key); + } + + /// Finds the key in the list and returns an iterator to the item. + /// + /// @return ConstIterator to the item in the list, will be equal to `cend()` when + /// the key isn't found + template + ConstIterator find(const TKey &key) const { + return skiplist_->find(key); } /// Finds the key or the first larger key in the list and returns an @@ -713,8 +752,18 @@ class SkipList final : detail::SkipListNode_base { /// @return Iterator to the item in the list, will be equal to `end()` when /// no items match the search template - Iterator find_equal_or_greater(const TKey &key) const { - return skiplist_->template find_equal_or_greater(key); + Iterator find_equal_or_greater(const TKey &key) { + return skiplist_->find_equal_or_greater(key); + } + + /// Finds the key or the first larger key in the list and returns an + /// iterator to the item. + /// + /// @return ConstIterator to the item in the list, will be equal to `end()` when + /// no items match the search + template + ConstIterator find_equal_or_greater(const TKey &key) const { + return skiplist_->find_equal_or_greater(key); } /// Estimates the number of items that are contained in the list that are @@ -727,7 +776,7 @@ class SkipList final : detail::SkipListNode_base { /// @return uint64_t estimated count of identical items in the list template uint64_t estimate_count(const TKey &key, int max_layer_for_estimation = kSkipListCountEstimateDefaultLayer) const { - return skiplist_->template estimate_count(key, max_layer_for_estimation); + return skiplist_->estimate_count(key, max_layer_for_estimation); } /// Estimates the number of items that are contained in the list that are @@ -742,7 +791,7 @@ class SkipList final : detail::SkipListNode_base { uint64_t estimate_range_count(const std::optional> &lower, const std::optional> &upper, int max_layer_for_estimation = kSkipListCountEstimateDefaultLayer) const { - return skiplist_->template estimate_range_count(lower, upper, max_layer_for_estimation); + return skiplist_->estimate_range_count(lower, upper, max_layer_for_estimation); } /// Estimates the average number of objects in the list that have the same @@ -759,7 +808,7 @@ class SkipList final : detail::SkipListNode_base { template uint64_t estimate_average_number_of_equals( const TCallable &equal_cmp, int max_layer_for_estimation = kSkipListCountEstimateDefaultLayer) const { - return skiplist_->template estimate_average_number_of_equals(equal_cmp, max_layer_for_estimation); + return skiplist_->estimate_average_number_of_equals(equal_cmp, max_layer_for_estimation); } /// Removes the key from the list. @@ -767,7 +816,7 @@ class SkipList final : detail::SkipListNode_base { /// @return bool indicating whether the removal was successful template bool remove(const TKey &key) { - return skiplist_->template remove(key); + return skiplist_->remove(key); } /// Returns the number of items contained in the list. @@ -787,6 +836,10 @@ class SkipList final : detail::SkipListNode_base { explicit ConstAccessor(const SkipList *skiplist) : skiplist_(skiplist), id_(skiplist->gc_.AllocateId()) {} public: + using value_type = TObj; + using iterator = ConstIterator; + using const_iterator = ConstIterator; + ~ConstAccessor() { if (skiplist_ != nullptr) skiplist_->gc_.ReleaseId(id_); } @@ -812,35 +865,35 @@ class SkipList final : detail::SkipListNode_base { template bool contains(const TKey &key) const { - return skiplist_->template contains(key); + return skiplist_->contains(key); } template ConstIterator find(const TKey &key) const { - return skiplist_->template find(key); + return skiplist_->find(key); } template ConstIterator find_equal_or_greater(const TKey &key) const { - return skiplist_->template find_equal_or_greater(key); + return skiplist_->find_equal_or_greater(key); } template uint64_t estimate_count(const TKey &key, int max_layer_for_estimation = kSkipListCountEstimateDefaultLayer) const { - return skiplist_->template estimate_count(key, max_layer_for_estimation); + return skiplist_->estimate_count(key, max_layer_for_estimation); } template uint64_t estimate_range_count(const std::optional> &lower, const std::optional> &upper, int max_layer_for_estimation = kSkipListCountEstimateDefaultLayer) const { - return skiplist_->template estimate_range_count(lower, upper, max_layer_for_estimation); + return skiplist_->estimate_range_count(lower, upper, max_layer_for_estimation); } template uint64_t estimate_average_number_of_equals( const TCallable &equal_cmp, int max_layer_for_estimation = kSkipListCountEstimateDefaultLayer) const { - return skiplist_->template estimate_average_number_of_equals(equal_cmp, max_layer_for_estimation); + return skiplist_->estimate_average_number_of_equals(equal_cmp, max_layer_for_estimation); } uint64_t size() const { return skiplist_->size(); } @@ -1018,26 +1071,33 @@ class SkipList final : detail::SkipListNode_base { } template - bool contains(const TKey &key) const { - TNode *preds[kSkipListMaxHeight], *succs[kSkipListMaxHeight]; - int layer_found = find_node(key, preds, succs); - return (layer_found != -1 && succs[layer_found]->fully_linked.load(std::memory_order_acquire) && - !succs[layer_found]->marked.load(std::memory_order_acquire)); - } - - template - Iterator find(const TKey &key) const { + SkipListNode *find_(const TKey &key) const { TNode *preds[kSkipListMaxHeight], *succs[kSkipListMaxHeight]; int layer_found = find_node(key, preds, succs); if (layer_found != -1 && succs[layer_found]->fully_linked.load(std::memory_order_acquire) && !succs[layer_found]->marked.load(std::memory_order_acquire)) { - return Iterator{succs[layer_found]}; + return succs[layer_found]; } - return Iterator{nullptr}; + return nullptr; } template - Iterator find_equal_or_greater(const TKey &key) const { + bool contains(const TKey &key) const { + return find_(key) != nullptr; + } + + template + Iterator find(const TKey &key) { + return {find_(key)}; + } + + template + ConstIterator find(const TKey &key) const { + return {find_(key)}; + } + + template + Iterator find_equal_or_greater_(const TKey &key) const { TNode *preds[kSkipListMaxHeight], *succs[kSkipListMaxHeight]; find_node(key, preds, succs); if (succs[0] && succs[0]->fully_linked.load(std::memory_order_acquire) && @@ -1047,6 +1107,16 @@ class SkipList final : detail::SkipListNode_base { return Iterator{nullptr}; } + template + Iterator find_equal_or_greater(const TKey &key) { + return {find_equal_or_greater_(key)}; + } + + template + ConstIterator find_equal_or_greater(const TKey &key) const { + return {find_equal_or_greater_(key)}; + } + template uint64_t estimate_count(const TKey &key, int max_layer_for_estimation) const { MG_ASSERT(max_layer_for_estimation >= 1 && max_layer_for_estimation <= kSkipListMaxHeight, diff --git a/tests/e2e/interactive_mg_runner.py b/tests/e2e/interactive_mg_runner.py old mode 100644 new mode 100755 index 13aa951db..f0e4e6da1 --- a/tests/e2e/interactive_mg_runner.py +++ b/tests/e2e/interactive_mg_runner.py @@ -1,3 +1,4 @@ +#!/usr/bin/env python3 # Copyright 2022 Memgraph Ltd. # # Use of this software is governed by the Business Source License diff --git a/tests/unit/skip_list.cpp b/tests/unit/skip_list.cpp index 86fb4ae57..bdebacc1e 100644 --- a/tests/unit/skip_list.cpp +++ b/tests/unit/skip_list.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2024 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -18,6 +18,20 @@ #include "utils/skip_list.hpp" #include "utils/timer.hpp" +#include + +template +concept CompatibleIterators = std::forward_iterator && std::forward_iterator && + requires(It it, ConstIt cit) { + { it == cit } -> std::same_as; + { it != cit } -> std::same_as; + { cit == it } -> std::same_as; + { cit != it } -> std::same_as; +}; + +using sut_t = memgraph::utils::SkipList::Accessor; +static_assert(CompatibleIterators); + TEST(SkipList, Int) { memgraph::utils::SkipList list; {