Am i correctly implementing Iterators for a my custom Vector struct?

code review
1

Hello, everyone
I have been working on my first rust project and the first part of this project was implementing a Vector struct.While writing the implementation for Vector i had some trouble implementing Iterators and after some struggle i got the code to work. I just wanted to see if this a good way to implement iterators for custom structs?

pub struct Vector<T: Float> {
    pub i: T, // i-hat component
    pub j: T, // j-hat component
    pub k: T, // k-hat component
}
pub struct Iter<'a, T: Float> {
    inner: &'a Vector<T>,
    index: u8
}

pub struct IntoIter<T: Float> {
    inner: Vector<T>,
    index: u8
}

pub struct IterMut<'a, T: Float> {
    inner: *mut Vector<T>,
    index: u8,
    _phantom: PhantomData<&'a mut Vector<T>>
}
impl<'a, T: Float> Iterator for Iter<'a, T> {
    type Item = &'a T;

    fn next(&mut self) -> Option<Self::Item> {
        let to_return = match self.index {
            0 => Some(&self.inner.i),
            1 => Some(&self.inner.j),
            2 => Some(&self.inner.k),
            _ => None
        };

        self.index += 1;

        to_return
    }
}

impl<T:Float> Iterator for IntoIter<T> {
    type Item = T;

    fn next(&mut self) -> Option<Self::Item> {
        let to_return = match self.index {
            0 => Some(self.inner.i),
            1 => Some(self.inner.j),
            2 => Some(self.inner.k),
            _ => None
        };

        self.index += 1;

        to_return

    }
}

impl<'a, T: Float> Iterator for IterMut<'a, T> {
    type Item = &'a mut T;

    fn next(&mut self) -> Option<Self::Item> {
        let to_return = match self.index {
            0 => Some( unsafe {&mut (*self.inner).i} ),
            1 => Some( unsafe {&mut (*self.inner).j} ),
            2 => Some( unsafe {&mut (*self.inner).k} ),
            _ => None
        };

        self.index += 1;

        to_return
    }
}

impl<'a, T: Float> IntoIterator for &'a Vector<T> {
    type Item = &'a T;
    type IntoIter = Iter<'a, T>;

    fn into_iter(self) -> Self::IntoIter {
        Iter{
            inner: self,
            index: 0
        }
    }
}

impl<T: Float> IntoIterator for Vector<T> {
    type Item = T;
    type IntoIter = IntoIter<T>;

    fn into_iter(self) -> Self::IntoIter {
        IntoIter {
            inner: self,
            index: 0
        }
    }
}

impl<'a, T: Float> IntoIterator for &'a mut Vector<T> {
    type Item = &'a mut T;
    type IntoIter = IterMut<'a, T>;

    fn into_iter(self) -> Self::IntoIter {
        IterMut{
            inner: self,
            index: 0,
            _phantom: PhantomData
        }
    }
}
2

It may be easier to simply leverage existing iterator implementations instead, like:

impl<T: Float> Vector<T> {
    pub fn iter(&self) -> impl Iterator<Item = &T> {
        [&self.i, &self.j, &self.k].into_iter()
    }
    
    pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut T> {
        [&mut self.i, &mut self.j, &mut self.k].into_iter()
    }
    
    pub fn into_iter(self) -> impl Iterator<Item = T> {
        [self.i, self.j, self.k].into_iter()
    }
}
5 Likes
3

Your implementation looks fine. Some improvements you might consider are:

  1. Don't put bounds (T: Float) on struct definition, but only on impl blocks. I guess that Float: Copy, so in iter implementation I would just bound T to be Copy.
  2. Document usage of raw pointers and unsafe. In your case it is required and sound, but it might be surprising to readers why you don't just store &mut Vector<T> in IterMut.
  3. Implement other auxiliary traits from std::iter module for your iterators. Notably FusedIterator, ExactSizeIterator and possibly DoubleEndedIterator.
4

None of your iterators are fused. They may yield surprising behavior after u8::MAX calls to .next().

Your could early-return None from the iterators or .saturating_add() to index.

1 Like
5

Unfortunately since you didn't implement the actual traits, you don't get the nice for loop sugar.

You can still do this, just until we get ITIAT you have to explicitly refer to the underlying std iterator type, like std::slice::Iter for slice iteration.

1 Like
6

That's a good point, and it basically could look like:

impl<T: Float> IntoIterator for Vector<T> {
    type Item = T;

    type IntoIter = <[T; 3] as IntoIterator>::IntoIter;
    
    fn into_iter(self) -> Self::IntoIter {
        [self.i, self.j, self.k].into_iter()
    }
}

The upside to this approach is that you get a whole lot of Iterator-adjacent trait implementations for free (ExactSizeIterator, DoubleEndedIterator, FusedIterator, etc.). The downside is that exposing what iterator we're using would mean it'd be a semver-breaking change if the number of fields changed (but since they're already pub, it probably would be anyway).

2 Likes