4

I have written a fancy "zip iterator" that already fulfils many roles (can be used in for_each, copy loops, container iterator range constructors etc...). Under all the template code to work around the pairs/tuples involved, it comes down to the dereference operator of the iterator returning a tuple/pair of references and not a reference to a tuple/pair.

I want my iterator to work with std::sort, so I need to be able to do swap(*iter1, *iter2) and have the underlying values switched in the original containers being iterated over.

The code and a small demo can be viewed here (it's quite a bit to get through): http://coliru.stacked-crooked.com/a/4fe23b4458d2e692

Although libstdc++'s sort uses std::iter_swap which calls swap, e.g. libc++'s does not, and it just calls swap directly, so I would like a solution involving swap as the customization point.

What I have tried (and gotten oooooh so close to working) is instead of returning std::pair/std::tuple from the operator* as I am doing now, is returning a simple wrapper type instead. The intent is to have the wrapper behave as if it were a std::pair/std::tuple, and allow me to write a swap function for it.

It looked like this:

template<typename... ValueTypes>
struct TupleWrapper : public PairOrTuple_t<ValueTypes...>
{
    using PairOrTuple_t<ValueTypes...>::operator=;
    template<typename... TupleValueTypes>
    operator PairOrTuple_t<TupleValueTypes...>() const
    {
       return static_cast<PairOrTuple_t<ValueTypes...>>(*this);
    }
};

template<std::size_t Index, typename... ValueTypes>
decltype(auto) get(TupleWrapper<ValueTypes...>& tupleWrapper)
{
    return std::get<Index>(tupleWrapper);
}
template<std::size_t Index, typename... ValueTypes>
decltype(auto) get(TupleWrapper<ValueTypes...>&& tupleWrapper)
{
    return std::get<Index>(std::forward<TupleWrapper<ValueTypes...>>(tupleWrapper));
}
template<typename... ValueTypes,
         std::size_t... Indices>
void swap(TupleWrapper<ValueTypes...> left,
          TupleWrapper<ValueTypes...> right,
          const std::index_sequence<Indices...>&)
{
    (std::swap(std::get<Indices>(left), std::get<Indices>(right)), ...);
}

template<typename... ValueTypes>
void swap(TupleWrapper<ValueTypes...> left,
          TupleWrapper<ValueTypes...> right)
{
    swap(left, right, std::make_index_sequence<sizeof...(ValueTypes)>());
}


namespace std
{
    template<typename... ValueTypes>
    class tuple_size<utility::implementation::TupleWrapper<ValueTypes...>> : public tuple_size<utility::implementation::PairOrTuple_t<ValueTypes...>> {};
    template<std::size_t Index, typename... ValueTypes>
    class tuple_element<Index, utility::implementation::TupleWrapper<ValueTypes...>> : public tuple_element<Index, utility::implementation::PairOrTuple_t<ValueTypes...>> {};
}

Full code here: http://coliru.stacked-crooked.com/a/951cd639d95af130.

Returning this wrapper in operator* seems to compile (at least on GCC) but produces garbage. On Clang's libc++, the std::tie fails to compile.

Two questions:

  1. How can I get this to compile with libc++ (the magic seems to lie in the conversion operator of TupleWrapper?)
  2. Why is the result wrong and what did I do wrong?

I know it's a lot of code, but well, I can't get it any shorter as all the tiny examples of swapping tuple wrappers worked fine for me.

rubenvb
  • 74,642
  • 33
  • 187
  • 332
  • Sorry but I dont' understand... what code doesn't compiler with clang++? The one that ends with `130`? – max66 Jul 28 '20 at 19:40
  • The basic issue is that sort can do things like move elements out of the range and back. It's not required to only use swaps. Things like `auto ele = std::move(*first);` simply doesn't work with your iterator. That's why C++20 has an `iter_move` customization point. – T.C. Jul 29 '20 at 03:51
  • @max66 Hmm, Here, Clang 5 seems to work fine (and deliver a correct result even for some reason): https://godbolt.org/z/7zc96z. On my Mac though, it doesn't want to compile with the actual code I took this excerpt from. I'll try to figure out the difference – rubenvb Jul 29 '20 at 10:45

1 Answers1

0

1st problem

One of the issues is that the ZipIterator class does not satisfy the requirements of RandomAccessIterator.

ForwardIterators have the condition that ::reference must be value_type& / const value_type&:

The type std::iterator_traits<It>::reference must be exactly

  • T& if It satisfies OutputIterator (It is mutable)
  • const T& otherwise (It is constant)

(where T is the type denoted by std::iterator_traits<It>::value_type)

which ZipIterator currently doesn't implement.

It works fine with std::for_each and similar functions that only require the iterator to satisfy the requirements of InputIterator / OutputIterator.

The reference type for an input iterator that is not also a LegacyForwardIterator does not have to be a reference type: dereferencing an input iterator may return a proxy object or value_type itself by value (as in the case of std::istreambuf_iterator).

tl;dr: ZipIterator can be used as an InputIterator / OutputIterator, but not as a ForwardIterator, which std::sort requires.

2nd problem

As @T.C. pointed out in their comment std::sort is allowed to move values out of the container and then later move them back in.

The type of dereferenced RandomIt must meet the requirements of MoveAssignable and MoveConstructible.

which ZipIterator currently can't handle (it never copies / moves the referenced objects), so something like this doesn't work as expected:

std::vector<std::string> vector_of_strings{"one", "two", "three", "four"};
std::vector<int> vector_of_ints{1, 2, 3, 4};
    
        
auto first = zipBegin(vector_of_strings, vector_of_ints);
auto second = first + 1; 
    
// swap two values via a temporary
auto temp = std::move(*first);
*first = std::move(*second);
*second = std::move(temp);

// Result:
/*
  two, 2
  two, 2
  three, 3
  four, 4
*/

(test on Godbolt)

Result

Unfortunately it is not possible to create an iterator that produces elements on the fly and can by used as a ForwardIterator with the current standard (for example this question)

  • You could of course write your own algorithms that only require InputIterators / OutputIterators (or handle your ZipIterator differently)
    For example a simple bubble sort: (Godbolt)
template<class It>
void bubble_sort(It begin, It end) {
    using std::swap;

    int n = std::distance(begin, end);

    for (int i = 0; i < n-1; i++) {   
        for (int j = 0; j < n-i-1; j++) {
        if (*(begin+j) > *(begin+j+1)) 
            swap(*(begin+j), *(begin+j+1)); 
        }
    }
}
  • Or change the ZipIterator class to satisfy RandomAccessIterator.
    I unfortunately can't think of a way that would be possible without putting the tuples into a dynamically allocated structure like an array (which you're probably trying to avoid)
Turtlefight
  • 9,420
  • 2
  • 23
  • 40