rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.72k stars 309 forks source link

Suggestion: map an iterator of indexes to their values in a slice #743

Open NightEule5 opened 1 year ago

NightEule5 commented 1 year ago

I've run into a pattern that I'm surprised isn't an iterator method or extension already (or if it is, I haven't found it):

let slice = b"Hello world!";
let range = 2..9;
for byte in range.map(|i| &slice[i]) {
    // ...
}

An map_index extension could be added for this, like:

struct MapIndex<'a, I, T> {
    iter: I,
    source: &'a T
}

impl<'a, I: Iterator, T: Index<I::Item>> Iterator for MapIndex<'a, I, T>
where T::Output: Sized + 'a {
    type Item = &'a T::Output;

    fn next(&mut self) -> Option<&'a T::Output> {
        self.iter.next().map(|index|
            &self.source[index]
        )
    }
}

trait Itertools: Iterator {
    fn map_index<T: Index<Self::Item>>(self, source: &T) -> MapIndex<Self, T>
    where T::Output: Sized,
          Self: Sized {
        MapIndex { iter: self, source }
    }
}
scottmcm commented 1 year ago

This seems entirely fine, though?

.map(|i| &slice[i])

Generally anything that's "this particular map" needs to be incredibly common to be worth adding. (See https://github.com/rust-itertools/itertools/issues/726#issuecomment-1701150877 for a recent example of something not being accepted.)

phimuemue commented 1 year ago

Hi there, I guess that would be covered by #447.

Philippe-Cholet commented 1 year ago

What's wrong with slice[range]?

let slice = b"Hello world!";
let range = 2..9;
for byte in &slice[range] {
    // ...
}

And range.map(|i| &slice[i]).collect() and slice[range].iter().collect() are equal.

EDIT: I guess when range is not a range but a more complex iterator of indexes?

NightEule5 commented 1 year ago

EDIT: I guess when range is not a range but a more complex iterator of indexes?

Yep, that was the intention. That was a bad example on my part. For an iterator like, say, [0, 2, 3, 1].into_iter(), or when indexing a type which doesn't support slicing, like VecDeque, you'd need map(|i| &values[i]). The title should've been "values in an Index impl" rather than "slice" specifically.

Generally anything that's "this particular map" needs to be incredibly common to be worth adding. (See https://github.com/rust-itertools/itertools/issues/726#issuecomment-1701150877 for a recent example of something not being accepted.)

Totally fair. I usually use slices for this too, but yesterday I was iterating over a range of VecDeque values (as mentioned above):

let buf: VecDeque<T> = ...;
let range: Range<usize> = ...;

for value in range.map(|i| &buf[i]) {
    // ...
}

This doesn't come up a lot for me either, I understand it not being common enough for this crate

Philippe-Cholet commented 1 year ago

It makes me think of Itertools::combinations (&co, that I'm currently editing) where a similar thing is done internally self.indices.iter().map(|i| self.pool[*i].clone()).collect(). I think it might be common enough. (The documentation should probably suggest to use slice[range] whenever possible.)

indices.map(|i| &buf[i])
indices.map_index(&buf)

map_index version would be nicer to type I guess (I don't like typing closures), but maybe map version is short enough. I think it's clearer than the proposed alternative (as it requires to know one more method). (Plus we are gonna prefix all methods with it_ soon leading to indices.it_map_index(&buf)). And I suppose it would not change/improve performance.