Skip to content

Comments

Make SmallVec::retain compatible with Vec::remain#89

Closed
aloucks wants to merge 1 commit intoservo:masterfrom
aloucks:std_vec_retain
Closed

Make SmallVec::retain compatible with Vec::remain#89
aloucks wants to merge 1 commit intoservo:masterfrom
aloucks:std_vec_retain

Conversation

@aloucks
Copy link

@aloucks aloucks commented Apr 1, 2018

Replaces &mut with & in the function predicate so that it's compatible with Vec::retain:

pub fn retain<F>(&mut self, f: F) 
where
    F: FnMut(&T) -> bool

This change is Reviewable

@tomaka
Copy link
Contributor

tomaka commented Apr 1, 2018

If I remember correctly, the designers of Vec::retain mentioned that it was a mistake on their side to use & and not &mut (and they cannot fix it because of backward compatibility). The &mut solution seems strictly superior to me.

@aloucks
Copy link
Author

aloucks commented Apr 1, 2018

@tomaka I don't necessarily disagree, but I ran into an issue where trying to write something generic over Vec and SmallVec and got hung up by the differences in mutability. I assumed the differences in the function signatures was an oversight.

@emilio
Copy link
Member

emilio commented Apr 1, 2018

mut seems better to me as well, Why does the generic code don't work? Could you put an example?

@aloucks
Copy link
Author

aloucks commented Apr 2, 2018

I want to create blanket impl's over newtypes where the inner component is either a Vec or a SmallVec. I was under the impression that SmallVec's interface should be compatible with Vec, but I agree that allowing the closure to take the item by &mut is a little more flexible. I'm able to work around it by copying the retain implementation, so it's not overly critical. Here's an example:

extern crate smallvec;

use smallvec::{SmallVec, Array};
use std::ops::{Deref, DerefMut, Rem};

macro_rules! tuple_deref {
    ($Type:ty, $Target:ty) => {
        impl Deref for $Type {
            type Target = $Target;
            fn deref(&self) -> &Self::Target {
                &self.0
            }
        }

        impl DerefMut for $Type {
            fn deref_mut(&mut self) -> &mut Self::Target {
                &mut self.0
            }
        }
    }
}

pub struct A(Vec<u32>);
pub struct B(SmallVec<[u32; 1]>);

tuple_deref!(A, Vec<u32>);
tuple_deref!(B, SmallVec<[u32; 1]>);

pub trait VecLike {
    type Item;
    fn is_empty(&self) -> bool;
    fn retain<F>(&mut self, f: F) where F: FnMut(&Self::Item) -> bool, Self: Sized;
}

impl<T> VecLike for Vec<T> {
    type Item = T;
    fn is_empty(&self) -> bool {
        Vec::<T>::is_empty(self)
    }
    fn retain<F>(&mut self, f: F) where F: FnMut(&Self::Item) -> bool, Self: Sized {
        Vec::<T>::retain(self, f)
    }
}

impl<T: Array> VecLike for SmallVec<T> {
    type Item = T::Item;
    fn is_empty(&self) -> bool {
        SmallVec::<T>::is_empty(self)
    }
    fn retain<F>(&mut self, mut f: F) where F: FnMut(&Self::Item) -> bool, Self: Sized {
        // SmallVec::<T>::retain(self, f)
        //    |
        // 51 |         SmallVec::<T>::retain(self, f)
        //    |         ^^^^^^^^^^^^^^^^^^^^^ the trait `for<'r> std::ops::FnMut<(&'r mut <T as smallvec::Array>::Item,)>` is not implemented for `F`
        //    |
        //    = help: consider adding a `where for<'r> F: std::ops::FnMut<(&'r mut <T as smallvec::Array>::Item,)>` bound
        //    = note: required by `<smallvec::SmallVec<A>>::retain`

        // https://docs.rs/smallvec/0.6.0/src/smallvec/lib.rs.html#637-648
        let mut del = 0;
        let len = self.len();
        for i in 0..len {
            if !f(&self[i]) {
                del += 1;
            } else if del > 0 {
                self.swap(i - del, i);
            }
        }
        self.truncate(len - del);
    }
}

pub trait Foobar<I> {
    fn foobar(&mut self);
}

impl<T, V, I> Foobar<I> for T 
    where V: VecLike<Item=I>,
          T: DerefMut<Target=V>,
          I: Copy + PartialEq<u32> + Rem<u32, Output=I>
{
    fn foobar(&mut self) {
        self.retain(|item| *item % 2 == 0);
    }
}

fn main() {
    let v = vec![1, 2, 3, 4];
    let mut a = A(v.clone());
    let mut b = B(SmallVec::<[u32; 1]>::from_vec(v.clone()));

    a.foobar();
    b.foobar();

    assert_eq!(a.as_slice(), &[2, 4]);
    assert_eq!(b.as_slice(), &[2, 4]);
}

@emilio
Copy link
Member

emilio commented Apr 2, 2018

@aloucks I think you can just do:

fn retain<F>(&mut self, mut f: F) where F: FnMut(&Self::Item) -> bool, Self: Sized {
    SmallVec::<T>::retain(self, |e| f(&*e))
}

@aloucks
Copy link
Author

aloucks commented Apr 2, 2018

@emilio Thanks, that's much cleaner. I thought I had tried that, but apparently I had not!

@aloucks aloucks closed this Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants