Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement Extend trait #142

wants to merge 2 commits into from

Conversation

SalsaGal
Copy link

@SalsaGal SalsaGal commented Feb 2, 2024

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.

src/lib.rs Outdated
Comment on lines 1203 to 1209
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);
}
}
}
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Author

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()
}

Copy link
Contributor

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.

Copy link

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>?

Copy link
Contributor

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.
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