Make generic_ functions taking &impl IntoIter more generic

[oberien]

The original functions could not take references to slices and were somewhat limiting in their input. This change makes them more generic by allowing more types to be passed into them.

While this is technically a breaking change, because code which explicitly states the generics won't compile anymore, all other code should be unaffected. Thus, I expect pretty much no code to break in the wild. A minor version increase might be a good idea anyways.

[maxbachmann]

Is there any reason why this uses:

a.clone().into_iter().count()

instead of

let a_iter = a.into_iter()
a_iter.clone().count()

I feel like the former might need to copy the underlying buffer, while the second only copies the iterator. I could be wrong about this though.

[oberien]

The current code takes a reference to an iterator. This PR allows additionally passing in anything that implements Iterator. If you pass in a reference to the iterator as previously, the Iterator + Clone bound willy be satisfied as the reference implements Clone, the reference.clone() code will clone the reference, not the underlying container.

If, however, you use the more generic trait bounds to directly pass in a container like Vec, then the full Vec will be cloned either way. In general foo.into_iter().clone() will usually have the same effect as foo.clone().into_iter() as IntoIterator::into_iter consumes self, indicating that the resulting iterator struct (usually called IntoIter) will likely wrap / contain the original collection. Thus, cloning that will also clone the underlying buffer.

[maxbachmann]

Hm I would have expected that worst case foo.into_iter().clone() does the same as foo.clone().into_iter(), since it stores the thing it's iterating over and essentially passes on the clone. However couldn't the iterator decide to use something like a refcounted storage internally? Not sure whether people actually do something like this in praxis though.

A second advantage that just came to my mind is that the iterator is a more specific type and so it allows us to avoid monomorphization by moving the implementation into a separate function and forwarding the iterator:
https://godbolt.org/z/dE4drsTqG

[maxbachmann]

Note that I could absolutely be wrong on these things, since I am pretty new to rust.

[oberien]

Sorry for the late response. There is a very simple reason that I require Iter1: Clone: Backwards compatibility. Currently, only &impl IntoIterator are allowed. By accepting impl IntoIterator + Clone, we still allow all &impl IntoIterator to be passed as &T implements Clone. But we can also pass in more types. If we were to require impl IntoIterator where Iter: Clone, not all previously allowed types can be passed in, as we add an additional requirement that wasn't there before. Thus, afaict this change only requires a minor version update.

If we ignore compatibility and focus only on performance, either can result in better performance, depending on the implementation.

• std::array::IntoIter contains both the array and a range. As such, if no element was consumed so far, we need to clone the range in addition to all array elements. If, however, elements were already consumed, only the unconsumed elements are cloned. In our case we clone the untouched IntoIter, which would be more expensive.
• std::slice::Iter & std::slice::IterMut contain the start-pointer and end-pointer of the iterated-over slice. A slice itself is a pointer and a length. Cloning a slice vs its iterator is equal performance-wise.
• std::vec::IntoIter contains 5 pointers / usize while Vec only has 3 (ptr, size, capacity). Cloning the IntoIter clones the elements as well as more pointers compared to cloning just the vec.

In the end, cloning a few dwords is negligible performance-wise. Either trait bound will in general result in very similar performance.

[maxbachmann]

This certainly makes sense. Internally we already run into this in the string versions. They use StringWrapper only to workaround this. I would consider this a breaking change though.