iterator: improve reference semantics

iterator.hpp

@@ -22,6 +22,7 @@
 #include "variadic.hpp"
 #include "view.hpp"
 
+#include <iterator>
 
 template <typename Base>
 class referencing_iterator {
@@ -283,115 +284,116 @@ namespace util {
 
     ///////////////////////////////////////////////////////////////////////////
     namespace detail::zip {
+        template <
+            typename IteratorT,
+            typename = std::make_index_sequence<std::tuple_size_v<IteratorT>>
+        >
+        struct iterator;
+
+
+        template <typename IteratorT, std::size_t ...Indices>
+        struct iterator<IteratorT, std::index_sequence<Indices...>> {
+        public:
+            // we cannot be a forward iterator because we don't want to supply
+            // references to a value type as that would necessitate storing
+            // said value types within the iterator.
+            using iterator_category = std::input_iterator_tag;
+            using difference_type = std::ptrdiff_t;
+
+            iterator (IteratorT _iterators):
+                m_iterators (_iterators)
+            { ; }
+
+
+            iterator&
+            operator++ (void)
+            {
+                std::tuple (++std::get<Indices> (m_iterators)...);
+                return *this;
+            }
+
+
+            iterator operator++ (int);
+
+            auto
+            operator* (void)
+            {
+                return std::forward_as_tuple (*std::get<Indices> (m_iterators)...);
+            }
+
+
+            bool
+            operator== (const iterator &rhs) const
+            {
+                return m_iterators == rhs.m_iterators;
+            }
+
+
+            bool
+            operator!= (const iterator &rhs) const
+            {
+                return !(*this == rhs);
+            }
+
+        private:
+            IteratorT m_iterators;
+        };
+
+
         // holds a tuple of iterators for begin and end, and returns an
         // iterator that transforms these iterators into tuples of value_types.
         //
         // this must be expressed in terms of iterators, rather than containers,
         // because it dramatically simplifies iterating over raw arrays.
         //
-        // IteratorT: a tuple of iterators across all containers
+        // we have to store begin and end iterators because we might not have
+        // enough information to determine the correct types from StoreT;
+        // eg, in the case of arrays that have decayed to pointers we can't
+        // find the end.
         //
-        // StoreT: a tuple of containers we own. used when we were provided
+        // BeginT: a tuple of begin iterators across all containers
-        //         with an rval at zip time. allows us to destroy the data
+        //
-        //         when we're actually done iterating.
+        // EndT: a tuple of end iterators across all containers
+        //
+        // StoreT: a tuple of containers we might own. used when we were
+        //         provided with an rval at zip time. allows us to destroy the
+        //         data when we're actually done iterating.
         template <
-            typename IteratorT,
             typename StoreT,
-            typename I = std::make_index_sequence<std::tuple_size<IteratorT>::value>
+            typename BeginT,
+            typename EndT,
+            typename I = std::make_index_sequence<std::tuple_size_v<StoreT>>
         >
         class collection;
 
 
         //---------------------------------------------------------------------
         template <
-            typename IteratorT,
             typename StoreT,
+            typename BeginT,
+            typename EndT,
             std::size_t ...I
         >
         class collection<
-            IteratorT,
             StoreT,
+            BeginT,
+            EndT,
             std::index_sequence<I...>
         > {
         public:
-            collection (IteratorT _begin, IteratorT _end, StoreT &&_store):
+            collection (StoreT _store, BeginT _begin, EndT _end):
-                m_begin { _begin },
+                m_store { std::move (_store) },
-                m_end   { _end   },
+                m_begin (std::move (_begin)),
-                m_store { std::forward<StoreT> (_store) }
+                m_end   (std::move (_end))
             { ; }
 
-            struct iterator : std::iterator<
+            auto begin (void)& { return iterator<BeginT> { m_begin }; }
-                std::forward_iterator_tag,
+            auto end   (void)& { return iterator<EndT> { m_end }; }
-                std::tuple<
-                    typename std::iterator_traits<
-                        typename std::tuple_element<I,IteratorT>::type
-                    >::value_type...
-                >,
-                std::size_t
-            > {
-            public:
-                iterator (IteratorT _iterators):
-                    m_iterators (_iterators)
-                { ; }
-
-
-                iterator&
-                operator++ (void)
-                {
-                    // HACK: we don't actually need to create a tuple here,
-                    // but it's a zero cost method to expand the parameter
-                    // pack.
-                    std::make_tuple (++std::get<I> (m_iterators)...);
-                    return *this;
-                }
-
-
-                iterator operator++ (int);
-
-
-                auto
-                operator* (void)
-                {
-                    return std::make_tuple (*std::get<I> (m_iterators)...);
-                }
-
-
-                bool
-                operator== (const iterator &rhs) const
-                {
-                    return m_iterators == rhs.m_iterators;
-                }
-
-
-                bool
-                operator!= (const iterator &rhs) const
-                {
-                    return !(*this == rhs);
-                }
-
-            private:
-                IteratorT m_iterators;
-            };
-
-
-            iterator
-            begin (void)
-            {
-                return iterator { { std::get<I> (m_begin)... } };
-            }
-
-
-            iterator
-            end (void)
-            {
-                return iterator { { std::get<I> (m_end)... } };
-            }
-
 
         private:
-            IteratorT m_begin;
+            StoreT m_store;
-            IteratorT m_end;
+            BeginT m_begin;
-            const StoreT m_store;
+            EndT   m_end;
         };
     }
 
@@ -408,19 +410,16 @@ namespace util {
     auto
     zip (ContainerT&&... data)
     {
-        using IteratorT = std::tuple<decltype(std::begin(data))...>;
-
-        auto store = util::variadic::filter<std::is_rvalue_reference> (std::forward<ContainerT> (data)...);
-
         return detail::zip::collection<
-            IteratorT,
+            decltype (std::forward_as_tuple (data...)),
-            decltype(store),
+            decltype (std::make_tuple (std::begin (data)...)),
+            decltype (std::make_tuple (std::end (data)...)),
             std::make_index_sequence<sizeof...(ContainerT)>
-        > {
+        > (
+            std::forward_as_tuple (data...),
             std::make_tuple (std::begin (data)...),
-            std::make_tuple (std::end   (data)...),
+            std::make_tuple (std::end (data)...)
-            std::move (store)
+        );
-        };
     };
 
 

test/iterator.cpp

@@ -12,18 +12,42 @@ main (int, char**)
 {
     util::TAP::logger tap;
 
-    std::vector<int> v_int { 1, 2, 3 };
+    // test that iteration across disparate types works as expected.
-    std::array<float,3> a_float { 1.1f, 2.2f, 3.3f };
+    //
-    char c_char[] = { 'a', 'b', 'c' };
+    // the char array is an important element because it tends to decay to a
+    // pointer type unless we're paying careful attention.
+    {
+        std::vector<int> v_int { 1, 2, 3 };
+        std::array<float,3> a_float { 1.1f, 2.2f, 3.3f };
+        char c_char[] = { '\0', 'b', 'c' };
 
-    bool success = true;
+        bool success = true;
-    for (const auto &[i, v, a, c]: util::izip (v_int, a_float, c_char)) {
+        for (auto [i, v, a, c]: util::izip (v_int, a_float, c_char)) {
-        success = success &&
+            success = success &&
-                  v_int[i] == v &&
+                      v_int[i] == v &&
-                  util::equal (a_float[i], a) &&
+                      util::equal (a_float[i], a) &&
-                  c_char[i] == c;
+                      c_char[i] == c;
+        }
+
+        tap.expect (success, "izip containers of int, float, and char and an initialiser_list");
+    }
+
+
+    // test that the obvious structured binding syntax gives references to
+    // the underlying values rather than silently supplying lvalues.
+    //
+    // we deliberately do not use any type of references in the range loop so
+    // that we check the syntax a user is likely to employ actually works.
+    // references may not be an obvious consideration.
+    {
+        const std::array<unsigned,3> src { 0, 1, 2 };
+        std::array<unsigned,3> dst { 2, 0, 1 };
+
+        for (auto [a,b]: util::zip (src, dst))
+            b = a;
+
+        tap.expect_eq (src, dst, "copy using structured bindings");
     }
 
-    tap.expect (success, "izip containers of int, float, and char and an initialiser_list");
     return tap.status ();
 }

test/string.cpp

@@ -30,8 +30,9 @@ main (int, char**)
     };
 
     util::view src { csv.c_str (), csv.size () };
-    for (const auto &[tok, expected]: util::zip (util::tokeniser (src, ','), TESTS))
+    for (auto &&[tok, expected]: util::zip (util::tokeniser (src, ','), TESTS)) {
         tap.expect (equal (tok, expected.value), "%s", expected.message);
+    }
 
     return tap.status ();
 }