Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants