-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add Iterator::collect_into and Iterator::collect_with #92982
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think this sets a bad example since it uses
Extend
and we have separate code paths forFromIterator
which are optimized for the fact that we're starting with an empty vec. It's kind of like doingWhich is a silly thing to do when you can just use
iter.collect()
For that reason I think the name also is suboptimal since it associates itself with
collect()
and notextend()
.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.
Should I put Vec::with_capacity(3) 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.
size_hint
already takes care of that in many cases, so it would only help in cases where the size hint of the iterator isn't accurate (e.g. when it contains afilter
). Or maybe when collecting into a collection with a custom allocator.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.
Ok, so if I understood correctly, I should use a
filter
(or some other adapter) in the examples that makessize_hint
not provide any useful info foriter.collect()
right?I could also mention that usually
iter.collect()
is preferred whensize_hint
is accurateThere 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.
Yes
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.
This lack of optimisation for the empty case is a bit of a worry in terms of whether
collect_with
is a good API at all. Wouldn't all of the good usages be better covered by the mutablecollect_into
? I had envisioned we could use.collect_with(Vec::new())
to replace the turbofish, but it seems that is almost never a good idea, or at least only applicable in certain circumstances. Now we would be creating a more ergonomic API whose use we would have to immediately discourage.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.
collect_with
can be very useful in order to avoid reallocation(s) whensize_hint
returns the default value.I agree that it is almost never a good idea. I also like more the turbofish syntax where applicable.
My idea for
collect_with
was a method to be used whensize_hint
is not helpful and the collection (in which the items are collected) does not already exists. This allows shorter code:I think we could specify it in the doc. However, someone could still misuse the method. Calling
Extend::extend_reserve
should, at least, fix the speed gap betweencollect()
and misusedcollect_with
. (not already implemented)Eventually, I realised
collect_with
can be implemented with a call tocollect_into
, should it be a better implementation?