sfackler/streaming-iterator

Add `get_mut`

Closed this issue · 6 comments

I'm not sure why wasn't fn get_mut(&mut self) -> Option<&mut Self::Item>; added already, but it would be great to have it if possible.

Not all streaming iterators can support that -- for example ConvertRef or MapRef don't have any mutable access. Maybe it could be in a new trait for compatible iterators to implement.

I'm looking into implementing this and have tried two approaches:

  • As explained here, have two different traits with the exact same functionality but returning a mutable reference
  • Add a trait that extends StreamingIterator and adds a get_mut, next_mut and map_deref_mut (perhaps others?)

In both cases add a ConvertMut and convert_mut function

I'm inclined to think that the second solution makes more sense: For most functions all it's really needed is a reference and that way a type can implement both traits easily without having to reimplement advance:

/// An interface for dealing with mutable streaming iterators.
pub trait StreamingIteratorMut: StreamingIterator {
   /// Returns a mutable reference to the current element of the iterator.
   ///
   /// The behavior of calling this method before `advance` has been called is unspecified.
   fn get_mut(&mut self) -> Option<&mut Self::Item>;


   /// Advances the iterator and returns the next value as a mutable reference.
   ///
   /// The behavior of calling this method after the end of the iterator has been reached is
   /// unspecified.
   ///
   /// The default implementation simply calls `advance` followed by `get_mut`.
   #[inline]
   fn next_mut(&mut self) -> Option<&mut Self::Item> {
       self.advance();
       (*self).get_mut()
   }

   /// Creates a regular, non-streaming iterator which transforms elements of this iterator by
   /// passing them mutably to a closure.
   #[inline]
   fn map_deref_mut<B, F>(self, f: F) -> MapDerefMut<Self, F>
   where
       Self: Sized,
       F: FnMut(&mut Self::Item) -> B,
   {
       MapDerefMut { it: self, f }
   }
}

The main disadvantage is that the mutable version of some functions that might be desirable like for example map_mut can't be implemented this way as MapMut would have to implement StreamingIterator as well but can't cause get returns a non mutable reference but the MatMut function expects a mutable one:

/// A streaming iterator which transforms the elements of a streaming iterator.
#[derive(Debug)]
pub struct MapMut<I, F> {
    it: I,
    f: F,
}

impl<I, B: ?Sized, F> StreamingIterator for MapMut<I, F>
where
    I: StreamingIteratorMut,
    F: Fn(&mut I::Item) -> &mut B,
{
    type Item = B;
    #[inline]
    fn get(&self) -> Option<&B> {
        let it = &self.it;
        let f = &self.f;

        // this is an error cause it is behind a non mutable borrow
        // it.get_mut().map(|current| (f)(current) as &B)
        
        // this is also an error cause get returns a non mutable reference but F expects a mutable one
        it.get().map(|current| (f)(current) as &B)
    }
}
``

How serendipitous, I was also just working on this. I went with your second style, an extension trait adding get_mut, next_mut, fold_mut, and for_each_mut, and another trait for double-ended next_back_mut and rfold_mut. Here's a comparison on top of my #22 changes:
cuviper/streaming-iterator@sources...cuviper:mutable

I hesitate to make too many new _mut methods, especially where they need new types, but map_deref_mut might be a good one indeed. I did add convert_mut and ConvertMut in mine too.

That's great!, yes I think most methods can do just fine with a non mutable reference. And yes I also added the mutable DoubleEnded trait but forgot to post it.

Here's the implementation for MapDerefMut although it's pretty straightforward:

/// A regular, non-streaming iterator which transforms the elements of a streaming iterator.
#[derive(Debug)]
pub struct MapDerefMut<I, F> {
    it: I,
    f: F,
}

impl<I, B, F> Iterator for MapDerefMut<I, F>
where
    I: StreamingIteratorMut,
    F: FnMut(&mut I::Item) -> B,
{
    type Item = B;

    #[inline]
    fn next(&mut self) -> Option<Self::Item> {
        self.it.next_mut().map(&mut self.f)
    }

    #[inline]
    fn size_hint(&self) -> (usize, Option<usize>) {
        self.it.size_hint()
    }
}

impl<I, B, F> DoubleEndedIterator for MapDerefMut<I, F>
where
    I: DoubleEndedStreamingIteratorMut,
    F: FnMut(&mut I::Item) -> B,
{
    #[inline]
    fn next_back(&mut self) -> Option<Self::Item> {
        self.it.next_back_mut().map(&mut self.f)
    }
}

We should try to at least share credit, since we already duplicated effort. 🙂

Would it work for you to send that MapDerefMut as a PR adding to my branch? Or we could do that as consecutive PRs here. I would also add a fold to your Iterator, which should be simple with self.it.fold_mut(...).

Nah no worries, feel free to use the code i posted here, you've contributed already to this project so it'll surely be faster for you to get your PR merged.

I also tried a fold_mut in the second version with separated traits for mut and not mut and I see how that can be useful if only for specializations of the Mutable iterators.