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

Smarter default implementation of Extend::extend_reserve #88761

Closed
wants to merge 2 commits into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Sep 8, 2021

Following up on #72631 (comment): there is a way for extend_reserve to perform a sensible reserve by default, without always needing to be overridden by every Extend impl, as long as extend checks its argument's size_hint as many already do. For example the following code in String's Extend impl:

impl Extend<char> for String {
fn extend<I: IntoIterator<Item = char>>(&mut self, iter: I) {
let iterator = iter.into_iter();
let (lower_bound, _) = iterator.size_hint();
self.reserve(lower_bound);
iterator.for_each(move |c| self.push(c));
}

In a future PR we can clean up handwritten implementations of extend_reserve which are no longer providing value over this default implementation.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 8, 2021
Copy link
Member Author

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used this in the past to work around lack of stable extend_reserve successfully, but it may be possible there are Extend implementations in which this would trigger a panic.

@cuviper
Copy link
Member

cuviper commented Sep 8, 2021

Iterator::size_hint docs say:

It is not enforced that an iterator implementation yields the declared number of elements. A buggy iterator may yield less than the lower bound or more than the upper bound of elements.

How comfortable are we with intentionally using a "buggy" iterator?

but it may be possible there are Extend implementations in which this would trigger a panic.

Funny, @scottmcm just demonstrated that in the users forum:
https://users.rust-lang.org/t/is-string-splitn-missing-a-huge-easy-optimisation/64555/3

@the8472
Copy link
Member

the8472 commented Sep 8, 2021

I have seen suggestions similar to the one in the urlo thread before. Since specialization isn't stable it might be used as a poor man's TrustedLen when the length is fixed.

@dtolnay
Copy link
Member Author

dtolnay commented Sep 9, 2021

I think I am going to withdraw this. While it works well in practice for a wide range of Extend impls, there is nothing that makes it incorrect for an Extend impl to panic in response to this situation, which would become a bad/surprising extend_reserve default behavior.

@dtolnay dtolnay closed this Sep 9, 2021
@dtolnay dtolnay deleted the extend_reserve branch September 9, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants