-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Extend trait #142
base: master
Are you sure you want to change the base?
Conversation
src/lib.rs
Outdated
impl<T> Extend<T> for Slab<T> { | ||
fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) { | ||
for value in iter.into_iter() { | ||
self.insert(value); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inserting values into a slab without saving the resulting indexes is usually not useful. Do you have a use-case for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that, I personally have a use case where the resulting indices aren't used, but it might be good to not use the trait at all and just have a function that returns Vec<usize>
or an iterator like that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this might work:
pub fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) -> Vec<usize> {
iter.into_iter().map(|x| self.insert(x)).collect()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think I prefer the original version. A Vec<usize>
will often not be the form that the user wants the indexes in.
I think it's okay to just say that users who want the indexes can iterate it themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about -> impl Iterator<Item=usize>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could make sense, but it would be a larger change. I don't think we can do it without a custom iterator.
Not having a return value allows us to follow the existing trait.
This function now returns Vec<usize> for the keys too.
This PR implements the
Extend
trait by just inserting all of the items in the iterator. I also added a test function for it, but I'm not sure how necessary that is.