Skip to content

Tracking issue for shrink_to feature #56431

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

Jump to bottom
Closed
2 of 3 tasks
ordovicia opened this issue Dec 2, 2018 · 9 comments · Fixed by #86879
Closed
2 of 3 tasks

Tracking issue for shrink_to feature #56431

ordovicia opened this issue Dec 2, 2018 · 9 comments · Fixed by #86879
Labels
A-collections Area: `std::collections` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ordovicia
Copy link
Contributor

ordovicia commented Dec 2, 2018 •
edited by yaahc
Loading

Steps

Questions

  • Does this need an RFC?

cc @Diggsey
@sfackler sfackler added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Dec 2, 2018
@kennytm
Copy link
Member

kennytm commented Dec 2, 2018

Someone needs to update BinaryHeap::shrink_to to point to this issue.

@ordovicia ordovicia mentioned this issue Dec 2, 2018
Merged
@ordovicia
Copy link
Contributor Author

I was just doing it 😄

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018
@Centril
Rollup merge of rust-lang#56432 - ordovicia:shrink-to-issue, r=Centril
c03c34c 
Update issue number of `shrink_to` methods to point the tracking issue

Tracking issue: rust-lang#56431
kennytm added a commit to kennytm/rust that referenced this issue Dec 3, 2018
@kennytm
Rollup merge of rust-lang#56432 - ordovicia:shrink-to-issue, r=Centril
52a4fc8 
Update issue number of `shrink_to` methods to point the tracking issue

Tracking issue: rust-lang#56431
@Amanieu
Copy link
Member

Amanieu commented Dec 5, 2018

Currently the shrink_to method will panic if the given capacity is greater than the current one. I think this should be changed to silently ignore such cases instead.

I can see shrink_to being particularly useful when you want to reuse an existing allocation for something different (for example another pass of a multi-pass algorithm). The container will usually be empty at this point and you just want to ensure that a large allocation used for one pass doesn't clog up memory for the rest of the program's lifetime.

@ErichDonGubler
Copy link
Contributor

I would agree with @Amanieu here -- I've just recently found a case where shrink_to would be handy, particularly if it silently no-op'd in the case of capacity actually being smaller than the specified maximum.

@Amanieu Amanieu mentioned this issue Apr 22, 2019
Merged
bors bot added a commit to rust-lang/hashbrown that referenced this issue Apr 22, 2019
@bors @Amanieu
Merge #67
7e2da14 
67: Change shrink_to to not panic if min_capacity < capacity r=Amanieu a=Amanieu

cc rust-lang/rust#56431

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
@Amanieu
Copy link
Member

Amanieu commented Apr 22, 2019

I have changed the behavior in hashbrown to no-op if the current capacity is smaller than the minimum (https://github.com/Amanieu/hashbrown/pull/67). There is currently still an assert in the libstd wrapper around hashbrown that panics though.

@In-line
Copy link
Contributor

In-line commented May 7, 2019 •
edited
Loading

IMHO I would prefer shrink_to not to panic. I think it is better to return Result<usize, TooSmallError> or maybe Option<usize>?

Ok(..) would contain previous capacity.
Err(...) will be returned in case of failure, containing required minimum.

With this change if I need code to panic, I will just unwrap(), but with current behavior it seems to be error prone.

@Mark-Simulacrum Mark-Simulacrum mentioned this issue Apr 8, 2020
Closed
@KodrAus KodrAus added A-collections Area: `std::collections` Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@thomwiggers thomwiggers mentioned this issue Jan 24, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 27, 2021
@bors
Auto merge of rust-lang#81335 - thomwiggers:no-panic-shrink-to, r=Mar…
a2f8f62 
…k-Simulacrum

Trying to shrink_to greater than capacity should be no-op

Per the discussion in rust-lang#56431, `shrink_to` shouldn't panic if you try to make a vector shrink to a capacity greater than its current capacity.
@marmeladema
Copy link
Contributor

I think #81335 fixed the last remaining concern.
Could it be stabilized, at least for Vec?
I am ready to do the PR if needed 👍

@plaidfinch
Copy link

@marmeladema I'd love for this API to be stabilized. Is there anything I can do to help out?

@Folyd
Copy link
Contributor

Folyd commented May 17, 2021

@kwf Feel free to file a stabilize PR. See: https://rustc-dev-guide.rust-lang.org/stabilization_guide.html#stabilization-pr

@YohDeadfall YohDeadfall mentioned this issue Jul 5, 2021
Merged
@bors bors closed this as completed in ad981d5 Aug 8, 2021
@g1mv g1mv mentioned this issue Jan 24, 2022
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
A-collections Area: `std::collections` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stabilize Vec<T>::shrink_to YohDeadfall/rust
11 participants
@kennytm @Amanieu @plaidfinch @ErichDonGubler @sfackler @marmeladema @Folyd @KodrAus @ordovicia @In-line and others