view: drop conceptually broken reverse iterators

docs/md/entity.md

@@ -2240,7 +2240,7 @@ See the relevant documentation for more information.
 
 A special mention is needed for the iterators returned by views and groups. Most
 of the times they meet the requirements of random access iterators, in all cases
-they meet at least the requirements of bidirectional iterators.<br/>
+they meet at least the requirements of forward iterators.<br/>
 In other terms, they are suitable for use with the parallel algorithms of the
 standard library. If it's not clear, this is a great thing.
 

src/entt/entity/view.hpp

@@ -81,10 +81,6 @@ public:
         ignore_as_empty_v<typename Storage::value_type>,
         iterable_storage_iterator<typename basic_common_type::iterator>,
         iterable_storage_iterator<typename basic_common_type::iterator, decltype(std::declval<Storage>().begin())>>;
-    using reverse_iterator = std::conditional_t<
-        ignore_as_empty_v<typename Storage::value_type>,
-        iterable_storage_iterator<typename basic_common_type::reverse_iterator>,
-        iterable_storage_iterator<typename basic_common_type::reverse_iterator, decltype(std::declval<Storage>().rbegin())>>;
 
     iterable_storage(Storage &ref)
         : pool{&ref} {}
@@ -97,14 +93,6 @@ public:
         return iterator{pool->basic_common_type::end(), pool->end()};
     }
 
-    [[nodiscard]] reverse_iterator rbegin() const ENTT_NOEXCEPT {
-        return reverse_iterator{pool->basic_common_type::rbegin(), pool->rbegin()};
-    }
-
-    [[nodiscard]] reverse_iterator rend() const ENTT_NOEXCEPT {
-        return reverse_iterator{pool->basic_common_type::rend(), pool->rend()};
-    }
-
 private:
     Storage *const pool;
 };
@@ -155,7 +143,6 @@ class iterable_view final {
 
 public:
     using iterator = iterable_view_iterator<typename View::iterator>;
-    using reverse_iterator = iterable_view_iterator<typename View::reverse_iterator>;
 
     iterable_view(const View &parent)
         : view{parent} {}
@@ -168,14 +155,6 @@ public:
         return {view.end(), &view};
     }
 
-    [[nodiscard]] reverse_iterator rbegin() const ENTT_NOEXCEPT {
-        return {view.rbegin(), &view};
-    }
-
-    [[nodiscard]] reverse_iterator rend() const ENTT_NOEXCEPT {
-        return {view.rend(), &view};
-    }
-
 private:
     const View view;
 };
@@ -194,19 +173,17 @@ public:
     using value_type = typename std::iterator_traits<It>::value_type;
     using pointer = typename std::iterator_traits<It>::pointer;
     using reference = typename std::iterator_traits<It>::reference;
-    using iterator_category = std::bidirectional_iterator_tag;
+    using iterator_category = std::forward_iterator_tag;
 
     view_iterator() ENTT_NOEXCEPT
-        : first{},
+        : it{},
           last{},
-          it{},
           pools{},
           filter{} {}
 
-    view_iterator(It from, It to, It curr, std::array<const Type *, Component> all_of, std::array<const Type *, Exclude> none_of) ENTT_NOEXCEPT
-        : first{from},
+    view_iterator(It curr, It to, std::array<const Type *, Component> all_of, std::array<const Type *, Exclude> none_of) ENTT_NOEXCEPT
+        : it{curr},
           last{to},
-          it{curr},
           pools{all_of},
           filter{none_of} {
         if(it != last && !valid()) {
@@ -224,16 +201,6 @@ public:
         return ++(*this), orig;
     }
 
-    view_iterator &operator--() ENTT_NOEXCEPT {
-        while(--it != first && !valid()) {}
-        return *this;
-    }
-
-    view_iterator operator--(int) ENTT_NOEXCEPT {
-        view_iterator orig = *this;
-        return operator--(), orig;
-    }
-
     [[nodiscard]] bool operator==(const view_iterator &other) const ENTT_NOEXCEPT {
         return other.it == it;
     }
@@ -251,9 +218,8 @@ public:
     }
 
 private:
-    It first;
-    It last;
     It it;
+    It last;
     std::array<const Type *, Component> pools;
     std::array<const Type *, Exclude> filter;
 };
@@ -354,8 +320,6 @@ public:
     using size_type = std::size_t;
     /*! @brief Bidirectional iterator type. */
     using iterator = internal::view_iterator<basic_common_type, typename basic_common_type::iterator, sizeof...(Component) - 1u, sizeof...(Exclude)>;
-    /*! @brief Reverse iterator type. */
-    using reverse_iterator = internal::view_iterator<basic_common_type, typename basic_common_type::reverse_iterator, sizeof...(Component) - 1u, sizeof...(Exclude)>;
     /*! @brief Iterable view type. */
     using iterable_view = internal::iterable_view<basic_view>;
     /*! @brief Common type among all storage types. */
@@ -453,7 +417,7 @@ public:
      * @return An iterator to the first entity of the view.
      */
     [[nodiscard]] iterator begin() const {
-        return iterator{view->begin(), view->end(), view->begin(), pools_to_array(std::index_sequence_for<Component...>{}), filter};
+        return iterator{view->begin(), view->end(), pools_to_array(std::index_sequence_for<Component...>{}), filter};
     }
 
     /**
@@ -466,34 +430,7 @@ public:
      * @return An iterator to the entity following the last entity of the view.
      */
     [[nodiscard]] iterator end() const {
-        return iterator{view->begin(), view->end(), view->end(), pools_to_array(std::index_sequence_for<Component...>{}), filter};
-    }
-
-    /**
-     * @brief Returns an iterator to the first entity of the reversed view.
-     *
-     * The returned iterator points to the first entity of the reversed view. If
-     * the view is empty, the returned iterator will be equal to `rend()`.
-     *
-     * @return An iterator to the first entity of the reversed view.
-     */
-    [[nodiscard]] reverse_iterator rbegin() const {
-        return reverse_iterator{view->rbegin(), view->rend(), view->rbegin(), pools_to_array(std::index_sequence_for<Component...>{}), filter};
-    }
-
-    /**
-     * @brief Returns an iterator that is past the last entity of the reversed
-     * view.
-     *
-     * The returned iterator points to the entity following the last entity of
-     * the reversed view. Attempting to dereference the returned iterator
-     * results in undefined behavior.
-     *
-     * @return An iterator to the entity following the last entity of the
-     * reversed view.
-     */
-    [[nodiscard]] reverse_iterator rend() const {
-        return reverse_iterator{view->rbegin(), view->rend(), view->rend(), pools_to_array(std::index_sequence_for<Component...>{}), filter};
+        return iterator{view->end(), view->end(), pools_to_array(std::index_sequence_for<Component...>{}), filter};
     }
 
     /**
@@ -512,8 +449,9 @@ public:
      * otherwise.
      */
     [[nodiscard]] entity_type back() const {
-        const auto it = rbegin();
-        return it != rend() ? *it : null;
+        auto it = view->rbegin();
+        for(const auto last = view->rend(); it != last && !contains(*it); ++it) {}
+        return it == view->rend() ? null : *it;
     }
 
     /**
@@ -523,8 +461,7 @@ public:
      * iterator otherwise.
      */
     [[nodiscard]] iterator find(const entity_type entt) const {
-        const auto it = iterator{view->begin(), view->end(), view->find(entt), pools_to_array(std::index_sequence_for<Component...>{}), filter};
-        return (it != end() && *it == entt) ? it : end();
+        return contains(entt) ? iterator{view->find(entt), view->end(), pools_to_array(std::index_sequence_for<Component...>{}), filter} : end();
     }
 
     /**
@@ -823,8 +760,7 @@ public:
      * otherwise.
      */
     [[nodiscard]] entity_type front() const {
-        const auto it = begin();
-        return it != end() ? *it : null;
+        return empty() ? null : *begin();
     }
 
     /**
@@ -833,8 +769,7 @@ public:
      * otherwise.
      */
     [[nodiscard]] entity_type back() const {
-        const auto it = rbegin();
-        return it != rend() ? *it : null;
+        return empty() ? null : *rbegin();
     }
 
     /**
@@ -844,8 +779,7 @@ public:
      * iterator otherwise.
      */
     [[nodiscard]] iterator find(const entity_type entt) const {
-        const auto it = view->find(entt);
-        return it != end() && *it == entt ? it : end();
+        return contains(entt) ? view->find(entt) : end();
     }
 
     /**

test/entt/entity/registry.cpp

@@ -567,7 +567,6 @@ TEST(Registry, RangeDestroy) {
     ASSERT_TRUE(registry.valid(entities[2u]));
 
     registry.destroy(icview.begin(), icview.end());
-    registry.destroy(icview.rbegin(), icview.rend());
 
     ASSERT_FALSE(registry.valid(entities[0u]));
     ASSERT_FALSE(registry.valid(entities[1u]));
@@ -1431,7 +1430,6 @@ TEST(Registry, Erase) {
 
     registry.erase<int, char>(entities[0u]);
     registry.erase<int, char>(icview.begin(), icview.end());
-    registry.erase<int, char>(icview.rbegin(), icview.rend());
 
     ASSERT_FALSE(registry.any_of<int>(entities[0u]));
     ASSERT_FALSE(registry.all_of<int>(entities[1u]));
@@ -1539,7 +1537,7 @@ TEST(Registry, Remove) {
     registry.remove<int, char>(entities[0u]);
 
     ASSERT_EQ((registry.remove<int, char>(icview.begin(), icview.end())), 2u);
-    ASSERT_EQ((registry.remove<int, char>(icview.rbegin(), icview.rend())), 0u);
+    ASSERT_EQ((registry.remove<int, char>(icview.begin(), icview.end())), 0u);
 
     ASSERT_FALSE(registry.any_of<int>(entities[0u]));
     ASSERT_FALSE(registry.all_of<int>(entities[1u]));
@@ -1602,7 +1600,7 @@ TEST(Registry, StableRemove) {
     registry.remove<int, stable_type>(entities[0u]);
 
     ASSERT_EQ((registry.remove<int, stable_type>(icview.begin(), icview.end())), 2u);
-    ASSERT_EQ((registry.remove<int, stable_type>(icview.rbegin(), icview.rend())), 0u);
+    ASSERT_EQ((registry.remove<int, stable_type>(icview.begin(), icview.end())), 0u);
 
     ASSERT_FALSE(registry.any_of<int>(entities[0u]));
     ASSERT_FALSE(registry.all_of<int>(entities[1u]));

test/entt/entity/view.cpp

@@ -171,48 +171,45 @@ TEST(SingleComponentView, Empty) {
 
 TEST(SingleComponentView, Each) {
     entt::registry registry;
+    entt::entity entity[2]{registry.create(), registry.create()};
 
-    registry.emplace<int>(registry.create(), 0);
-    registry.emplace<int>(registry.create(), 1);
+    registry.emplace<int>(entity[0u], 0);
+    registry.emplace<int>(entity[1u], 1);
 
     auto view = registry.view<int>();
+    auto cview = std::as_const(registry).view<const int>();
+
     auto iterable = view.each();
+    auto citerable = cview.each();
 
-    ASSERT_NE(iterable.begin(), iterable.end());
+    ASSERT_NE(citerable.begin(), citerable.end());
     ASSERT_NO_THROW(iterable.begin()->operator=(*iterable.begin()));
 
-    auto cview = std::as_const(registry).view<const int>();
-    auto citerable = cview.each();
+    auto it = iterable.begin();
 
-    std::size_t cnt = 0;
+    ASSERT_EQ((it++, ++it), iterable.end());
 
-    for(auto first = citerable.rbegin(), last = citerable.rend(); first != last; ++first) {
-        static_assert(std::is_same_v<decltype(*first), std::tuple<entt::entity, const int &>>);
-        ASSERT_EQ(std::get<1>(*first), cnt++);
-    }
+    view.each([expected = 1u](auto entt, int &value) mutable {
+        ASSERT_EQ(entt::to_integral(entt), expected);
+        ASSERT_EQ(value, expected);
+        --expected;
+    });
 
-    view.each([&cnt](auto, int &) { ++cnt; });
-    view.each([&cnt](int &) { ++cnt; });
+    cview.each([expected = 1u](const int &value) mutable {
+        ASSERT_EQ(value, expected);
+        --expected;
+    });
 
-    ASSERT_EQ(cnt, std::size_t{6});
+    ASSERT_EQ(std::get<0>(*iterable.begin()), entity[1u]);
+    ASSERT_EQ(std::get<0>(*++citerable.begin()), entity[0u]);
 
-    cview.each([&cnt](const int &) { --cnt; });
-    cview.each([&cnt](auto, const int &) { --cnt; });
+    static_assert(std::is_same_v<decltype(std::get<1>(*iterable.begin())), int &>);
+    static_assert(std::is_same_v<decltype(std::get<1>(*citerable.begin())), const int &>);
 
     // do not use iterable, make sure an iterable view works when created from a temporary
-    for(auto [entt, iv]: registry.view<int>().each()) {
-        static_assert(std::is_same_v<decltype(entt), entt::entity>);
-        static_assert(std::is_same_v<decltype(iv), int &>);
-        ASSERT_EQ(iv, --cnt);
+    for(auto [entt, value]: view.each()) {
+        ASSERT_EQ(entt::to_integral(entt), value);
     }
-
-    ASSERT_EQ(cnt, std::size_t{0});
-
-    auto it = iterable.begin();
-    auto rit = iterable.rbegin();
-
-    ASSERT_EQ((it++, ++it), iterable.end());
-    ASSERT_EQ((rit++, ++rit), iterable.rend());
 }
 
 TEST(SingleComponentView, ConstNonConstAndAllInBetween) {
@@ -493,19 +490,15 @@ TEST(MultiComponentView, Functionalities) {
     registry.emplace<char>(e1, '2');
 
     ASSERT_EQ(*view.begin(), e1);
-    ASSERT_EQ(*view.rbegin(), e1);
+    ASSERT_EQ(*cview.begin(), e1);
     ASSERT_EQ(++view.begin(), (view.end()));
-    ASSERT_EQ(++view.rbegin(), (view.rend()));
+    ASSERT_EQ(++cview.begin(), (cview.end()));
 
     ASSERT_NO_FATAL_FAILURE((view.begin()++));
     ASSERT_NO_FATAL_FAILURE((++cview.begin()));
-    ASSERT_NO_FATAL_FAILURE(view.rbegin()++);
-    ASSERT_NO_FATAL_FAILURE(++cview.rbegin());
 
     ASSERT_NE(view.begin(), view.end());
     ASSERT_NE(cview.begin(), cview.end());
-    ASSERT_NE(view.rbegin(), view.rend());
-    ASSERT_NE(cview.rbegin(), cview.rend());
     ASSERT_EQ(view.size_hint(), 1u);
 
     for(auto entity: view) {
@@ -597,9 +590,10 @@ TEST(MultiComponentView, LazyExcludedTypeFromConstRegistry) {
 
 TEST(MultiComponentView, Iterator) {
     entt::registry registry;
-    const auto entity = registry.create();
-    registry.emplace<int>(entity);
-    registry.emplace<char>(entity);
+    const entt::entity entity[2]{registry.create(), registry.create()};
+
+    registry.insert<int>(std::begin(entity), std::end(entity));
+    registry.insert<char>(std::begin(entity), std::end(entity));
 
     const auto view = registry.view<int, char>();
     using iterator = typename decltype(view)::iterator;
@@ -613,57 +607,13 @@ TEST(MultiComponentView, Iterator) {
     ASSERT_EQ(end, view.end());
     ASSERT_NE(begin, end);
 
+    ASSERT_EQ(*begin, entity[1u]);
+    ASSERT_EQ(*begin.operator->(), entity[1u]);
     ASSERT_EQ(begin++, view.begin());
-    ASSERT_EQ(begin--, view.end());
 
+    ASSERT_EQ(*begin, entity[0u]);
+    ASSERT_EQ(*begin.operator->(), entity[0u]);
     ASSERT_EQ(++begin, view.end());
-    ASSERT_EQ(--begin, view.begin());
-
-    ASSERT_EQ(*begin, entity);
-    ASSERT_EQ(*begin.operator->(), entity);
-
-    registry.emplace<int>(registry.create());
-    registry.emplace<char>(registry.create());
-
-    const auto other = registry.create();
-    registry.emplace<int>(other);
-    registry.emplace<char>(other);
-
-    begin = view.begin();
-
-    ASSERT_EQ(*(begin++), other);
-    ASSERT_EQ(*(begin++), entity);
-    ASSERT_EQ(begin--, end);
-    ASSERT_EQ(*(begin--), entity);
-    ASSERT_EQ(*begin, other);
-}
-
-TEST(MultiComponentView, ReverseIterator) {
-    entt::registry registry;
-    const auto entity = registry.create();
-    registry.emplace<int>(entity);
-    registry.emplace<char>(entity);
-
-    const auto view = registry.view<int, char>();
-    using iterator = typename decltype(view)::reverse_iterator;
-
-    iterator end{view.rbegin()};
-    iterator begin{};
-    begin = view.rend();
-    std::swap(begin, end);
-
-    ASSERT_EQ(begin, view.rbegin());
-    ASSERT_EQ(end, view.rend());
-    ASSERT_NE(begin, end);
-
-    ASSERT_EQ(begin++, view.rbegin());
-    ASSERT_EQ(begin--, view.rend());
-
-    ASSERT_EQ(++begin, view.rend());
-    ASSERT_EQ(--begin, view.rbegin());
-
-    ASSERT_EQ(*begin, entity);
-    ASSERT_EQ(*begin.operator->(), entity);
 }
 
 TEST(MultiComponentView, ElementAccess) {
@@ -718,59 +668,56 @@ TEST(MultiComponentView, SizeHint) {
 
     ASSERT_EQ(view.size_hint(), 1u);
     ASSERT_EQ(view.begin(), view.end());
-    ASSERT_EQ(view.rbegin(), view.rend());
 }
 
 TEST(MultiComponentView, Each) {
     entt::registry registry;
+    entt::entity entity[2]{registry.create(), registry.create()};
 
-    const auto e0 = registry.create();
-    registry.emplace<int>(e0, 0);
-    registry.emplace<char>(e0);
+    registry.emplace<int>(entity[0u], 0);
+    registry.emplace<char>(entity[0u], 0);
 
     const auto e1 = registry.create();
-    registry.emplace<int>(e1, 1);
-    registry.emplace<char>(e1);
+    registry.emplace<int>(entity[1u], 1);
+    registry.emplace<char>(entity[1u], 1);
 
     auto view = registry.view<int, char>();
+    auto cview = std::as_const(registry).view<const int, const char>();
+
     auto iterable = view.each();
+    auto citerable = cview.each();
 
-    ASSERT_NE(iterable.begin(), iterable.end());
+    ASSERT_NE(citerable.begin(), citerable.end());
     ASSERT_NO_THROW(iterable.begin()->operator=(*iterable.begin()));
 
-    auto cview = std::as_const(registry).view<const int, const char>();
-    auto citerable = cview.each();
+    auto it = iterable.begin();
 
-    std::size_t cnt = 0;
+    ASSERT_EQ((it++, ++it), iterable.end());
 
-    for(auto first = citerable.rbegin(), last = citerable.rend(); first != last; ++first) {
-        static_assert(std::is_same_v<decltype(*first), std::tuple<entt::entity, const int &, const char &>>);
-        ASSERT_EQ(std::get<1>(*first), cnt++);
-    }
+    view.each([expected = 1u](auto entt, int &ivalue, char &cvalue) mutable {
+        ASSERT_EQ(entt::to_integral(entt), expected);
+        ASSERT_EQ(ivalue, expected);
+        ASSERT_EQ(cvalue, expected);
+        --expected;
+    });
 
-    view.each([&cnt](auto, int &, char &) { ++cnt; });
-    view.each([&cnt](int &, char &) { ++cnt; });
+    cview.each([expected = 1u](const int &ivalue, const char &cvalue) mutable {
+        ASSERT_EQ(ivalue, expected);
+        ASSERT_EQ(cvalue, expected);
+        --expected;
+    });
 
-    ASSERT_EQ(cnt, std::size_t{6});
+    ASSERT_EQ(std::get<0>(*iterable.begin()), entity[1u]);
+    ASSERT_EQ(std::get<0>(*++citerable.begin()), entity[0u]);
 
-    cview.each([&cnt](const int &, const char &) { --cnt; });
-    cview.each([&cnt](auto, const int &, const char &) { --cnt; });
+    static_assert(std::is_same_v<decltype(std::get<1>(*iterable.begin())), int &>);
+    static_assert(std::is_same_v<decltype(std::get<2>(*citerable.begin())), const char &>);
 
     // do not use iterable, make sure an iterable view works when created from a temporary
-    for(auto [entt, iv, cv]: registry.view<int, char>().each()) {
-        static_assert(std::is_same_v<decltype(entt), entt::entity>);
-        static_assert(std::is_same_v<decltype(iv), int &>);
-        static_assert(std::is_same_v<decltype(cv), char &>);
-        ASSERT_EQ(iv, --cnt);
+    for(auto [entt, ivalue, cvalue]: registry.view<int, char>().each()) {
+        ASSERT_EQ(entt::to_integral(entt), ivalue);
+        ASSERT_EQ(entt::to_integral(entt), cvalue);
     }
-
-    ASSERT_EQ(cnt, std::size_t{0});
-
-    auto it = iterable.begin();
-    auto rit = iterable.rbegin();
-
-    ASSERT_EQ((it++, ++it), iterable.end());
-    ASSERT_EQ((rit++, ++rit), iterable.rend());
 }
 
 TEST(MultiComponentView, EachWithSuggestedType) {