iterator: rewrite detail::zip to avoid dangling references

iterator.hpp

@@ -6,11 +6,10 @@
  * Copyright 2010-2018 Danny Robson <danny@nerdcruft.net>
  */
 
-
+#pragma once
-#ifndef CRUFT_UTIL_ITERATOR_HPP
-#define CRUFT_UTIL_ITERATOR_HPP
 
 #include "types/traits.hpp"
+#include "tuple/value.hpp"
 #include "variadic.hpp"
 #include "view.hpp"
 
@@ -287,6 +286,10 @@ namespace cruft {
 
     ///////////////////////////////////////////////////////////////////////////
     namespace detail::zip {
+        /// A container that holds multiple iterators and returns a tuple of
+        /// their results when dereferenced.
+        ///
+        /// \tparam IteratorT  A tuple-like object that contains iterators
         template <
             typename IteratorT,
             typename = std::make_index_sequence<std::tuple_size_v<IteratorT>>
@@ -297,9 +300,13 @@ namespace cruft {
         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
+            // We can't declare ourselves as a forward_iterator because we're
-            // references to a value type as that would necessitate storing
+            // unable to supply references to our value_type when we get
-            // said value types within the iterator.
+            // dereferenced unless we store the supplied values/references
+            // inside ourself.
+            //
+            // This complicates implementation too much given we have no
+            // pressing need for this functionality.
             using iterator_category = std::input_iterator_tag;
             using difference_type = std::ptrdiff_t;
 
@@ -311,17 +318,26 @@ namespace cruft {
             iterator&
             operator++ (void)
             {
-                std::tuple (++std::get<Indices> (m_iterators)...);
+                (++std::get<Indices> (m_iterators), ...);
                 return *this;
             }
 
 
             iterator operator++ (int);
 
+
             auto
             operator* (void)
             {
-                return std::forward_as_tuple (*std::get<Indices> (m_iterators)...);
+                // We deliberately construct a tuple manually here to reduce
+                // the risk of dangling references. `forward_as_tuple` and
+                // `make_tuple` have resulted in references to locals in the
+                // past.
+                return std::tuple<
+                    decltype(*std::get<Indices> (m_iterators))...
+                > (
+                    *std::get<Indices> (m_iterators)...
+                );
             }
 
 
@@ -338,74 +354,67 @@ namespace cruft {
                 return !(*this == rhs);
             }
 
+
         private:
             IteratorT m_iterators;
         };
 
 
-        // holds a tuple of iterators for begin and end, and returns an
+        /// A simple container for multiple collections that returns a
-        // iterator that transforms these iterators into tuples of value_types.
+        /// wrapped multi-iterator for begin and end.
-        //
+        ///
-        // this must be expressed in terms of iterators, rather than containers,
+        /// It is up to the user to ensure StoreT does not contain dangling
-        // because it dramatically simplifies iterating over raw arrays.
+        /// references.
-        //
+        ///
-        // we have to store begin and end iterators because we might not have
+        /// \tparam StoreT  A tuple-like object of collections (or references
-        // enough information to determine the correct types from StoreT;
+        ///                 to collections).
-        // eg, in the case of arrays that have decayed to pointers we can't
+        template <typename ...StoreT>
-        // find the end.
+        class collection {
-        //
-        // BeginT: a tuple of begin iterators across all containers
-        //
-        // 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 StoreT,
-            typename BeginT,
-            typename EndT,
-            typename I = std::make_index_sequence<std::tuple_size_v<StoreT>>
-        >
-        class collection;
-
-
-        //---------------------------------------------------------------------
-        template <
-            typename StoreT,
-            typename BeginT,
-            typename EndT,
-            std::size_t ...I
-        >
-        class collection<
-            StoreT,
-            BeginT,
-            EndT,
-            std::index_sequence<I...>
-        > {
         public:
-            collection (StoreT _store, BeginT _begin, EndT _end):
+            collection (StoreT&&... _store):
-                m_store { std::move (_store) },
+                m_store (std::forward<StoreT> (_store)...)
-                m_begin (std::move (_begin)),
-                m_end   (std::move (_end))
             { ; }
 
-            auto begin (void)& { return iterator<BeginT> { m_begin }; }
+
-            auto end   (void)& { return iterator<EndT>   { m_end   }; }
+            using indices_t = std::make_index_sequence<sizeof...(StoreT)>;
+            using begin_t = std::tuple<decltype(std::begin (std::declval<StoreT> ()))...>;
+            using end_t   = std::tuple<decltype(std::end   (std::declval<StoreT> ()))...>;
+
+
+            auto begin (void)&
+            {
+                return iterator<begin_t, indices_t> (
+                    tuple::value::map (
+                        [] (auto &i) noexcept { return std::begin (i); },
+                        m_store
+                    )
+                );
+            }
+
+
+            auto end (void)&
+            {
+                return iterator<end_t, indices_t> (
+                    tuple::value::map (
+                        [] (auto &i) noexcept { return std::end (i); },
+                        m_store
+                    )
+                );
+            }
+
 
         private:
-            StoreT m_store;
+            std::tuple<StoreT...> m_store;
-            BeginT m_begin;
-            EndT   m_end;
         };
     }
 
 
     ///------------------------------------------------------------------------
-    /// takes a variable number of container arguments and returns an interable
+    /// Takes a variable number of container arguments and returns an interable
-    /// object with a value_type of tuple of the argument's value_types.
+    /// object with a value_type that is a tuple of the each container's
+    /// value_type.
     ///
-    /// the returned iterator value_type is suitable for using in range-for
+    /// The returned iterator value_type is suitable for using in range-for
     /// and structured bindings (and really, that's the entire point here).
     ///
     /// eg, cruft::zip ({1,2,3}, {4,5,6}) ~= {{1,4},{2,5},{3,6}}
@@ -415,32 +424,27 @@ namespace cruft {
     {
         CHECK (((std::size (data) == std::size (variadic::get<0> (data...))) && ...));
 
-        return detail::zip::collection<
+        return detail::zip::collection<ContainerT...> (
-            decltype (std::forward_as_tuple (data...)),
+            std::forward<ContainerT> (data)...
-            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)...)
         );
-    };
+    }
 
 
     ///------------------------------------------------------------------------
-    /// takes a variable number of containers and returns a zipped iterable
+    /// Takes a variable number of containers and returns a zipped iterable
     /// object where the first of the iterator's value_types is the index of
-    /// that iterator. ie, it combines container offsets with value_types.
+    /// that iterator. ie, It combines container offsets with value_types.
     ///
     /// eg, cruft::izip ("abc") ~= {{0,'a'},{1,'b'},{2,'c'}}
     template <typename ...ContainerT>
     auto
-    izip (ContainerT&... data)
+    izip (ContainerT&&... data)
     {
+        indices idx (::cruft::variadic::get<0> (data...));
+
         return zip (
-            indices (::cruft::variadic::get<0> (data...)),
+            std::move (idx),
-            data...
+            std::forward<ContainerT> (data)...
         );
     }
 
@@ -733,5 +737,3 @@ namespace cruft {
         IteratorT m_cursor;
     };
 }
-
-#endif