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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for shrink_to feature #56431

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: A tracking issue for an RFC or an unstable feature. 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: A tracking issue for an RFC or an unstable feature. 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 <[email protected]>
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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: A tracking issue for an RFC or an unstable feature. 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