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

Implementation for rust-lang/libs-team#138 #105072

Closed
wants to merge 4 commits into from
Closed

Implementation for rust-lang/libs-team#138 #105072

wants to merge 4 commits into from

Conversation

Sp00ph
Copy link
Member

@Sp00ph Sp00ph commented Nov 29, 2022

(See rust-lang/libs-team#138)

@scottmcm mentioned in the linked ACP that he'd be tempted to go straight to stable with these changes, as they don't include any complicated new compile-time functionality, so here I am, shooting my shot. If I instead should go through the process of first making it unstably const and then make it stable in a later PR, then I can also change this PR to do that.

r? @dtolnay

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 29, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@Sp00ph
Copy link
Member Author

Sp00ph commented Nov 29, 2022

@rustbot label +T-libs-api -T-libs

@rustbot rustbot 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 Nov 29, 2022
@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 29, 2022
@Sp00ph @scottmcm
Use CURRENT_RUSTC_VERSION placeholder
82fc730 
Co-authored-by: scottmcm <scottmcm@users.noreply.github.com>
fmease
fmease reviewed Nov 29, 2022
View reviewed changes
library/alloc/src/collections/vec_deque/mod.rs
pub fn new() -> VecDeque<T> {
VecDeque::new_in(Global)
pub const fn new() -> VecDeque<T> {
// FIXME: This should just be `VecDeque::new_in(Global)` once that's stable.
Copy link
Member

Choose a reason for hiding this comment

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

You could also slap #[allow_internal_unstable(allocator_api)] onto VecDeque::new instead. Not sure if that's frowned upon though. What does the library team say?

@inquisitivecrystal inquisitivecrystal added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 30, 2022
dtolnay
dtolnay requested changes Dec 1, 2022
View reviewed changes
Copy link
Member

@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.

Thank you @Sp00ph. The contents of the PR look good, but could I please get you to split this into 3 independent PRs?

  • The VecDeque::new const stabilization. This requires team approval but it will just sail through, there is no reason this would be controversial.

  • Adding const to the unstable VecDeque::new_in. This does not need team approval because it's a change to an unstable API. I can unilaterally approve this. We can stick your comment fix inside make_contiguous in here too.

  • The From conversion performance guarantee. A new stable guarantee requires team approval and this one needs more careful consideration than your const changes. It will be more convenient for the team if these can be considered independently.

This was referenced Dec 1, 2022
Merged
Merged
Merged
@Sp00ph
Copy link
Member Author

Sp00ph commented Dec 1, 2022

@dtolnay I opened a PR for each of those changes as you can see. Should I r? you in them?

@dtolnay
Copy link
Member

dtolnay commented Dec 1, 2022

Great!

@dtolnay dtolnay closed this Dec 1, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 2, 2022
@matthiaskrgr
Rollup merge of rust-lang#105126 - Sp00ph:const_new_in, r=dtolnay
4c4dec4 
Make `VecDeque::new_in` unstably const

(See rust-lang#105072)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2022
@bors
Auto merge of rust-lang#105127 - Sp00ph:const_new, r=dtolnay
1d12c3c 
Make `VecDeque::new` const

(See rust-lang#105072)
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#105127 - Sp00ph:const_new, r=dtolnay
b043cc3 
Make `VecDeque::new` const

(See rust-lang#105072)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 7, 2023
@matthiaskrgr
Rollup merge of rust-lang#105128 - Sp00ph:vec_vec_deque_conversion, r…
d7519c3 
…=dtolnay

Add O(1) `Vec -> VecDeque` conversion guarantee

(See rust-lang#105072)
@est31 est31 mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm left review comments

@fmease fmease fmease left review comments

@dtolnay dtolnay dtolnay requested changes

Assignees

@dtolnay dtolnay

Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 this pull request may close these issues.
6 participants
@Sp00ph @rustbot @dtolnay @fmease @scottmcm @inquisitivecrystal