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.

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 Box<[T; N]>: TryFrom<Vec<T>> #101837

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Sep 15, 2022

We have [T; N]: TryFrom<Vec<T>> (#76310) and Box<[T; N]>: TryFrom<Box<[T]>>, but not this combination.

vec.into_boxed_slice().try_into() isn't quite a replacement for this, as that'll reallocate unnecessarily in the error case.

Insta-stable, so needs an FCP

(I tried to make this work with , A, but that's disallowed because of #[fundamental] #29635 (comment))

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 15, 2022
@rustbot

This comment was marked as resolved.

@rust-highfive

This comment was marked as outdated.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2022
@scottmcm
Copy link
Member Author

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

And a coin flip between the two -api members on the review rotation says…
r? @m-ou-se

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2022
@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 15, 2022
@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 15, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Sep 15, 2022

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Sep 15, 2022

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

No concerns currently listed.

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 Sep 15, 2022
We have `[T; N]: TryFrom<Vec<T>>` and `Box<[T; N]>: TryFrom<Box<[T]>>`, but not the combination.

`vec.into_boxed_slice().try_into()` isn't quite a replacement for this, as that'll reallocate unnecessarily in the error case.

**Insta-stable, so needs an FCP**
@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 Sep 27, 2022
@rfcbot
Copy link

rfcbot commented Sep 27, 2022

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 7, 2022
@rfcbot
Copy link

rfcbot commented Oct 7, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Oct 7, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Oct 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2022

📌 Commit 4d3a31c has been approved by m-ou-se

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 17, 2022
@bors
Copy link
Contributor

bors commented Oct 17, 2022

⌛ Testing commit 4d3a31c with merge 06f049a...

@bors
Copy link
Contributor

bors commented Oct 17, 2022

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 06f049a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2022
@bors bors merged commit 06f049a into rust-lang:master Oct 17, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 17, 2022
@scottmcm scottmcm deleted the box-array-from-vec branch October 17, 2022 23:22
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (06f049a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.5% [2.2%, 2.9%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@jplatte
Copy link
Contributor

jplatte commented Nov 11, 2022

Is it really a good idea to stabilize this when that likely makes it impossible to generalize it even if fundamental gets "downgraded" to "fundamental-in-T" for Box down the road?

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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
Development

Successfully merging this pull request may close these issues.

10 participants