From b625ff090237dbd46df1c9b2f27e99502da9e779 Mon Sep 17 00:00:00 2001 From: Michele Caini Date: Wed, 20 Oct 2021 11:44:48 +0200 Subject: [PATCH] dense_hash_map: iterator review --- src/entt/container/dense_hash_map.hpp | 115 ++++++++++++++----------- test/entt/container/dense_hash_map.cpp | 24 ++++-- 2 files changed, 83 insertions(+), 56 deletions(-) diff --git a/src/entt/container/dense_hash_map.hpp b/src/entt/container/dense_hash_map.hpp index 7e6a4ef8b..fb86def8c 100644 --- a/src/entt/container/dense_hash_map.hpp +++ b/src/entt/container/dense_hash_map.hpp @@ -45,6 +45,7 @@ class dense_hash_map_iterator { using iterator_traits = std::iterator_traits()->element))>; public: + using iterator_type = It; using value_type = typename iterator_traits::value_type; using pointer = typename iterator_traits::pointer; using reference = typename iterator_traits::reference; @@ -53,11 +54,11 @@ public: dense_hash_map_iterator() ENTT_NOEXCEPT = default; - dense_hash_map_iterator(const It iter) ENTT_NOEXCEPT + dense_hash_map_iterator(const iterator_type iter) ENTT_NOEXCEPT : it{iter} {} - template>, typename = std::enable_if_t> - dense_hash_map_iterator(const dense_hash_map_iterator> *> &other) + template>, typename = std::enable_if_t> + dense_hash_map_iterator(const dense_hash_map_iterator> *> &other) : it{other.it} {} dense_hash_map_iterator &operator++() ENTT_NOEXCEPT { @@ -96,38 +97,10 @@ public: return (*this + -value); } - difference_type operator-(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return it - other.it; - } - [[nodiscard]] reference operator[](const difference_type value) const { return it->element; } - [[nodiscard]] bool operator==(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return it == other.it; - } - - [[nodiscard]] bool operator!=(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return !(*this == other); - } - - [[nodiscard]] bool operator<(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return it < other.it; - } - - [[nodiscard]] bool operator>(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return it > other.it; - } - - [[nodiscard]] bool operator<=(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return !(*this > other); - } - - [[nodiscard]] bool operator>=(const dense_hash_map_iterator &other) const ENTT_NOEXCEPT { - return !(*this < other); - } - [[nodiscard]] pointer operator->() const { return std::addressof(it->element); } @@ -136,10 +109,49 @@ public: return *operator->(); } + [[nodiscard]] iterator_type base() const ENTT_NOEXCEPT { + return it; + } + private: - It it; + iterator_type it; }; +template +[[nodiscard]] auto operator-(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return lhs.base() - rhs.base(); +} + +template +[[nodiscard]] bool operator==(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return lhs.base() == rhs.base(); +} + +template +[[nodiscard]] bool operator!=(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return !(lhs == rhs); +} + +template +[[nodiscard]] bool operator<(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return lhs.base() < rhs.base(); +} + +template +[[nodiscard]] bool operator>(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return lhs.base() > rhs.base(); +} + +template +[[nodiscard]] bool operator<=(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return !(lhs > rhs); +} + +template +[[nodiscard]] bool operator>=(const dense_hash_map_iterator &lhs, const dense_hash_map_iterator &rhs) ENTT_NOEXCEPT { + return !(lhs < rhs); +} + template class dense_hash_map_local_iterator { friend dense_hash_map_local_iterator *>; @@ -147,6 +159,7 @@ class dense_hash_map_local_iterator { using iterator_traits = std::iterator_traits()->element))>; public: + using iterator_type = It; using value_type = typename iterator_traits::value_type; using pointer = typename iterator_traits::pointer; using reference = typename iterator_traits::reference; @@ -155,12 +168,12 @@ public: dense_hash_map_local_iterator() ENTT_NOEXCEPT = default; - dense_hash_map_local_iterator(It iter, const std::size_t pos) ENTT_NOEXCEPT + dense_hash_map_local_iterator(iterator_type iter, const std::size_t pos) ENTT_NOEXCEPT : it{iter}, curr{pos} {} - template>, typename = std::enable_if_t> - dense_hash_map_local_iterator(const dense_hash_map_local_iterator> *> &other) + template>, typename = std::enable_if_t> + dense_hash_map_local_iterator(const dense_hash_map_local_iterator> *> &other) : it{other.it}, curr{other.curr} {} @@ -173,14 +186,6 @@ public: return ++(*this), orig; } - [[nodiscard]] bool operator==(const dense_hash_map_local_iterator &other) const ENTT_NOEXCEPT { - return curr == other.curr; - } - - [[nodiscard]] bool operator!=(const dense_hash_map_local_iterator &other) const ENTT_NOEXCEPT { - return !(*this == other); - } - [[nodiscard]] pointer operator->() const { return std::addressof(it[curr].element); } @@ -189,15 +194,25 @@ public: return *operator->(); } - [[nodiscard]] auto index() const ENTT_NOEXCEPT { - return curr; + [[nodiscard]] iterator_type base() const ENTT_NOEXCEPT { + return (it + curr); } private: - It it; + iterator_type it; std::size_t curr; }; +template +[[nodiscard]] bool operator==(const dense_hash_map_local_iterator &lhs, const dense_hash_map_local_iterator &rhs) ENTT_NOEXCEPT { + return lhs.base() == rhs.base(); +} + +template +[[nodiscard]] bool operator!=(const dense_hash_map_local_iterator &lhs, const dense_hash_map_local_iterator &rhs) ENTT_NOEXCEPT { + return !(lhs == rhs); +} + } // namespace internal /** @@ -239,7 +254,7 @@ class dense_hash_map final { [[nodiscard]] auto constrained_find(const Other &key, std::size_t bucket) { for(auto it = begin(bucket), last = end(bucket); it != last; ++it) { if(packed.second()(it->first, key)) { - return iterator{packed.first().data() + it.index()}; + return iterator{it.base()}; } } @@ -250,7 +265,7 @@ class dense_hash_map final { [[nodiscard]] auto constrained_find(const Other &key, std::size_t bucket) const { for(auto it = begin(bucket), last = end(bucket); it != last; ++it) { if(packed.second()(it->first, key)) { - return const_iterator{packed.first().data() + it.index()}; + return const_iterator{it.base()}; } } @@ -802,7 +817,7 @@ public: * @return An iterator to the end of the given bucket. */ [[nodiscard]] const_local_iterator cend([[maybe_unused]] const size_type index) const { - return {nullptr, std::numeric_limits::max()}; + return {packed.first().data(), std::numeric_limits::max()}; } /** @@ -820,7 +835,7 @@ public: * @return An iterator to the end of the given bucket. */ [[nodiscard]] local_iterator end([[maybe_unused]] const size_type index) { - return {nullptr, std::numeric_limits::max()}; + return {packed.first().data(), std::numeric_limits::max()}; } /** diff --git a/test/entt/container/dense_hash_map.cpp b/test/entt/container/dense_hash_map.cpp index 8b32c40dd..ea17d3fd3 100644 --- a/test/entt/container/dense_hash_map.cpp +++ b/test/entt/container/dense_hash_map.cpp @@ -279,6 +279,15 @@ TEST(DenseHashMap, IteratorConversion) { ASSERT_EQ((*it).second, 42); ASSERT_EQ(it->first, cit->first); ASSERT_EQ((*it).second, (*it).second); + + ASSERT_EQ(it - cit, 0); + ASSERT_EQ(cit - it, 0); + ASSERT_LE(it, cit); + ASSERT_LE(cit, it); + ASSERT_GE(it, cit); + ASSERT_GE(cit, it); + ASSERT_EQ(it, cit); + ASSERT_NE(++cit, it); } TEST(DenseHashMap, Insert) { @@ -899,11 +908,11 @@ TEST(DenseHashMap, LocalIterator) { ASSERT_EQ(begin->first, 3u + expected_bucket_count); ASSERT_EQ((*begin).second, 99u); - ASSERT_EQ(begin.index(), 1u); + ASSERT_EQ(begin.base(), map.begin().base() + 1u); ASSERT_EQ(begin++, map.begin(3u)); - ASSERT_EQ(begin.index(), 0u); + ASSERT_EQ(begin.base(), map.begin().base()); ASSERT_EQ(++begin, map.end(3u)); - ASSERT_GT(begin.index(), map.size()); + ASSERT_NE(begin.base(), map.end().base()); } TEST(DenseHashMap, ConstLocalIterator) { @@ -930,11 +939,11 @@ TEST(DenseHashMap, ConstLocalIterator) { ASSERT_EQ(cbegin->first, 3u + expected_bucket_count); ASSERT_EQ((*cbegin).second, 99u); - ASSERT_EQ(cbegin.index(), 1u); + ASSERT_EQ(cbegin.base(), map.cbegin().base() + 1u); ASSERT_EQ(cbegin++, map.begin(3u)); - ASSERT_EQ(cbegin.index(), 0u); + ASSERT_EQ(cbegin.base(), map.cbegin().base()); ASSERT_EQ(++cbegin, map.end(3u)); - ASSERT_GT(cbegin.index(), map.size()); + ASSERT_NE(cbegin.base(), map.cend().base()); } TEST(DenseHashMap, LocalIteratorConversion) { @@ -951,6 +960,9 @@ TEST(DenseHashMap, LocalIteratorConversion) { ASSERT_EQ((*it).second, 42); ASSERT_EQ(it->first, cit->first); ASSERT_EQ((*it).second, (*it).second); + + ASSERT_EQ(it, cit); + ASSERT_NE(++cit, it); } TEST(DenseHashMap, Rehash) {