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

Stabilize try_reserve #87993

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Stabilize try_reserve #87993

merged 1 commit into from
Oct 5, 2021

Conversation

kornelski
Copy link
Contributor

Stabilization PR for the try_reserve feature.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Aug 12, 2021
@jonas-schievink jonas-schievink added relnotes Marks issues that should be documented in the release notes of the next release. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 12, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@TimDiekmann
Copy link
Member

cc @rust-lang/wg-allocators

@the8472 the8472 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 13, 2021
@bors
Copy link
Contributor

bors commented Aug 18, 2021

☔ The latest upstream changes (presumably #86860) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member

r? @joshtriplett perhaps, not sure what current T-libs FCP practice is. I don't think I see a completed one though.

@joshtriplett
Copy link
Member

FCP for stabilization:
@rfcbot merge

@rfcbot
Copy link

rfcbot commented Aug 20, 2021 •
edited
Loading

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 20, 2021
@dtolnay
Copy link
Member

dtolnay commented Aug 21, 2021

@rfcbot concern need From<LayoutError> for TryReserveError? #48043 (comment)

@joshtriplett
Copy link
Member

@dtolnay Thanks for catching that. I don't think we need that From instance.

@kornelski kornelski mentioned this pull request Aug 22, 2021
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 26, 2021
@Dylan-DPC
Rollup merge of rust-lang#88216 - kornelski:from_layout_err, r=kennytm
b8f7072 
Don't stabilize creation of TryReserveError instances

rust-lang#48043 + rust-lang#87993 (comment)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 26, 2021
@GuillaumeGomez
Rollup merge of rust-lang#88216 - kornelski:from_layout_err, r=kennytm
b937bd4 
Don't stabilize creation of TryReserveError instances

rust-lang#48043 + rust-lang#87993 (comment)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 26, 2021
@Dylan-DPC
Rollup merge of rust-lang#88216 - kornelski:from_layout_err, r=kennytm
9a9af0d 
Don't stabilize creation of TryReserveError instances

rust-lang#48043 + rust-lang#87993 (comment)
@dtolnay
Copy link
Member

dtolnay commented Aug 26, 2021

@rfcbot resolve need From<LayoutError> for TryReserveError? #48043 (comment)
@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Aug 26, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 26, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 26, 2021
@Manishearth
Rollup merge of rust-lang#88216 - kornelski:from_layout_err, r=kennytm
cf5e362 
Don't stabilize creation of TryReserveError instances

rust-lang#48043 + rust-lang#87993 (comment)
@bors
Copy link
Contributor

bors commented Oct 4, 2021

📌 Commit 00152d8 has been approved by joshtriplett

@bors
Copy link
Contributor

bors commented Oct 4, 2021

🌲 The tree is currently closed for pull requests below priority 4. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
@Manishearth
Rollup merge of rust-lang#87993 - kornelski:try_reserve_stable, r=jos…
6ac923d 
…htriplett

Stabilize try_reserve

Stabilization PR for the [`try_reserve` feature](rust-lang#48043 (comment)).
@Manishearth Manishearth mentioned this pull request Oct 5, 2021
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
@Manishearth
Rollup merge of rust-lang#87993 - kornelski:try_reserve_stable, r=jos…
3cbd39f 
…htriplett

Stabilize try_reserve

Stabilization PR for the [`try_reserve` feature](rust-lang#48043 (comment)).
@Manishearth Manishearth mentioned this pull request Oct 5, 2021
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021
@Manishearth
Rollup merge of rust-lang#87993 - kornelski:try_reserve_stable, r=jos…
279cb9c 
…htriplett

Stabilize try_reserve

Stabilization PR for the [`try_reserve` feature](rust-lang#48043 (comment)).
@Manishearth Manishearth mentioned this pull request Oct 5, 2021
Closed
@workingjubilee workingjubilee mentioned this pull request Oct 5, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2021
@bors
Auto merge of rust-lang#89545 - workingjubilee:rollup-ooxf3p2, r=work…
a804c4b 
…ingjubilee

Rollup of 15 pull requests

Successful merges:

 - rust-lang#87993 (Stabilize try_reserve)
 - rust-lang#88090 (Perform type inference in range pattern)
 - rust-lang#88780 (Added abs_diff for integer types.)
 - rust-lang#89270 (path.push() should work as expected on windows verbatim paths)
 - rust-lang#89413 (Correctly handle supertraits for min_specialization)
 - rust-lang#89456 (Update to the final LLVM 13.0.0 release)
 - rust-lang#89466 (Fix bug with query modifier parsing)
 - rust-lang#89473 (Fix extra `non_snake_case` warning for shorthand field bindings)
 - rust-lang#89474 (rustdoc: Improve doctest pass's name and module's name)
 - rust-lang#89478 (Fixed numerus of error message)
 - rust-lang#89480 (Add test for issue 89118.)
 - rust-lang#89487 (Try to recover from a `=>` -> `=` or `->` typo in a match arm)
 - rust-lang#89494 (Deny `where` clauses on `auto` traits)
 - rust-lang#89511 (:arrow_up: rust-analyzer)
 - rust-lang#89536 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 99e6e3f into rust-lang:master Oct 5, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 5, 2021
@kornelski kornelski deleted the try_reserve_stable branch October 5, 2021 10:06
@jplatte jplatte mentioned this pull request Oct 5, 2021
Open
65 tasks
@ojeda ojeda mentioned this pull request Oct 16, 2021
Open
83 tasks
@h-vetinari h-vetinari mentioned this pull request Oct 18, 2021
Open
5 tasks
@TennyZhuang TennyZhuang mentioned this pull request Dec 4, 2021
Merged
@cmazakas
Copy link

cmazakas commented Dec 6, 2021

Why wasn't the actual error kind stabilized? There's no point to this feature without the ability to introspect.

@kornelski
Copy link
Contributor Author

kornelski commented Dec 6, 2021 •
edited
Loading

@LeonineKing1199 You can see in the original RFC discussion that it wasn't certain if the error should be stabilized exactly as it is implemented currently, with separate cases that just mirror an inconsistency in std (where some allocation failures panic, and some abort). Waiting for resolution of that question was delaying stabilization of the entire feature.

I'm interested to hear what is your use-case and what data you'd like to get out of the error type, to help guide how the error type should be designed.

@cmazakas
Copy link

cmazakas commented Dec 6, 2021

Ha ha, looks like my case was already addressed:

One I can think of is someone creating a custom data structure, and wanting to mimic std API. But that's not a very strong case.

It is for implementing the stdlib APIs without polluting it with polyfills that are incompatible with what's in the stdlib.

@kornelski
Copy link
Contributor Author

kornelski commented Dec 7, 2021 •
edited
Loading

Great. So you're not looking to get any data out of it yourself, you just want to return a TryReserveError instance? If there was TryReserveError::default() or just a very basic TryReserveError::new(allocation_size), would that work for you?

For now a hacky way to get some instance is Vec::<u8>::new().try_reserve(usize::MAX).unwrap_err(). That's guaranteed to always fail fast.

@Stargateur
Copy link
Contributor

Why wasn't the actual error kind stabilized? There's no point to this feature without the ability to introspect.

In my opinion, the error is more for the end user, or very special case for something like kernel linux. For a server I will log the error and continue anyway so don't really care, even if I would consider a CapacityOverflow a code bug in a server or at least get a look at it.

@kornelski Do you think we could have const value TryReserveError::CAPACITY_OVERFLOW ? Unless if we want to be able to precise the maximum value at runtine and possibly the value used I don't expect this to change a lot.

@cmazakas
Copy link

cmazakas commented Dec 7, 2021

Great. So you're not looking to get any data out of it yourself, you just want to return a TryReserveError instance? If there was TryReserveError::default() or just a very basic TryReserveError::new(allocation_size), would that work for you?

Actually, this would work perfectly!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2021
@matthiaskrgr
Rollup merge of rust-lang#91529 - TennyZhuang:try_reserve_binary_heap…
0a98071 
…, r=yaahc

add BinaryHeap::try_reserve and BinaryHeap::try_reserve_exact

`try_reserve` of many collections were stablized in rust-lang#87993 in 1.57.0. Add `try_reserve` for the rest collections such as `BinaryHeap` should be not controversial.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2021
@matthiaskrgr
Rollup merge of rust-lang#91529 - TennyZhuang:try_reserve_binary_heap…
7ec00cd 
…, r=yaahc

add BinaryHeap::try_reserve and BinaryHeap::try_reserve_exact

`try_reserve` of many collections were stablized in rust-lang#87993 in 1.57.0. Add `try_reserve` for the rest collections such as `BinaryHeap` should be not controversial.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

@Wodann Wodann Wodann requested changes

Assignees

@joshtriplett joshtriplett

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.