-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 implementations for converting boxed slices into boxed arrays #61515
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
use core::convert::TryFrom; | ||
|
||
#[derive(Debug, Copy, Clone)] | ||
#[unstable(feature = "boxed_slice_try_from", issue = "0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an issue (if deemed acceptable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: trait implementations are insta-stable.
Are you not able to parameterise over all array lengths? This doesn't requite const generics as it currently stands. |
I'm not following your meaning. This PR adds one more case of using macros to explicitly enumerate certain sizes. If this were to be merged before #61415, I assume that you'd want to update that PR to include this new code. If that PR is merged first, I should update this to follow suit. It didn't make sense to jump right to const generics in this PR until the decisions were made in the other PRs. I also wanted to prevent me adding anything complicated like the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat. I think you should be able to replicate this for Rc
and Arc
as well.
src/liballoc/boxed.rs
Outdated
|
||
fn try_from(boxed_slice: Box<[T]>) -> Result<Box<[T; $N]>, Self::Error> { | ||
if boxed_slice.len() == $N { | ||
unsafe { Ok(Box::from_raw(Box::into_raw(boxed_slice) as *mut [T; $N])) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment re. safety would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to copy the rationale from slices, but it didn't have one; neither does the unsafe
block immediately before this implementation in the file.
Is it valid to say
This is safe for the same reasons and under the same conditions that converting a slice to an array is safe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would elaborate more from the POV of the unsafe operations you are using and how you have verified the proofs they need. Also, Ok(unsafe {
seems preferable since the Ok
bit doesn't partake in the unsafety.
If we were adding a new, unstable method, it might make sense to use const generics, because there aren't concerns with breaking stable code. But these are insta-stable (#55436), so that's probably not advisable. |
src/liballoc/boxed.rs
Outdated
#[unstable(feature = "boxed_slice_try_from", issue = "0")] | ||
/// Can we use TryFromSliceError instead? | ||
/// TODO: implement Error | ||
pub struct PlaceholderError(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to create a custom error in order to recover the original allocation in case of failure.
99b30fb
to
8bea15e
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
8bea15e
to
9359367
Compare
☔ The latest upstream changes (presumably #61739) made this pull request unmergeable. Please resolve the merge conflicts. |
ping from triage @cramertj waiting for your review on this |
It seems like there are a number of unresolved questions still remaining above, and some unchecked checkboxes in the initial post. Also, now that #62435 has landed, we probably follow that pattern here as well rather than generating all the impls using a macro. |
Yes, but I didn’t wish to waste effort polishing a PR that a reviewer would reject; I was hoping for a “yes, this makes sense” or “no, let’s not do this”.
Yes, which I was hoping a reviewer would lend feedback on. |
Yes, I think it makes sense to have |
Thanks @shepmaster! I would personally agree that we should have these sorts of conversions. We already have I'm not really sure what the discussion for the safety rationale is (these seem trivially safe) but in terms of the return type I could personally go either way. It'd be a lot simpler to define and grok if the base value wasn't returned through the error type, but that means failed Unfortunately I think though these are insta-stable impls so we need to figure this out now, so I'd be curious to hear what others think. I'd slightly lean towards leaving out the original value in the error type. |
That is inconsistent with if a.len() == 2 {
do_work_on_two_array(*a.try_into().unwrap());
}
fn do_work_on_two_array<T>(_: [T; 2]){} Notice the unwrap. |
@bjorn3 I don’t understand why the decision to return the original value or not would affect the I see that I left some comments about using That means I’m really asking about these two versions:
The latter could be generalized for |
All in all, I would be in favor of: impl<T, const N: usize> TryFrom<Box<[T]>> for Box<[T; N]>
where
[T; N]: LengthAtMost32,
{
type Error = Box<[T]>;
// …
} |
9359367
to
c585ae6
Compare
Ah, I hadn't thought about returning the I've updated the PR to use const generics, as the referenced const generic PRs have been merged since this was opened. |
Please extend the UI tests Scott added to check that implementations for size e.g. 33 and some placeholder |
ping from triage @shepmaster any updates on this? |
c585ae6
to
f0cb1ca
Compare
Changes:
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
f0cb1ca
to
912b5bf
Compare
☔ The latest upstream changes (presumably #63207) made this pull request unmergeable. Please resolve the merge conflicts. |
This mirrors the implementations of reference slices into arrays.
912b5bf
to
32324d2
Compare
Conflicts fixed. |
@bors r+ I think this is good to go based on the comments above and my own review. Anyone from @rust-lang/libs feel free to r- if you see opportunities for improvement. |
📌 Commit 32324d2 has been approved by |
Add implementations for converting boxed slices into boxed arrays This mirrors the implementations of reference slices into arrays. # Discussion - [x] Should we use const generics? ([probably not](#61515 (comment))) - [ ] [What's the safety rationale here](#61515 (comment))? - [ ] [Should the errors return the original object](#61515 (comment))? # Remaining - [ ] Implement `Error` - [ ] Create a tracking issue
#61515 (comment) is still not resolved @bors r- |
@bors retry |
@bors: r=cramertj @Centril please don't r- for not religiously documenting all |
📌 Commit 32324d2 has been approved by |
Add implementations for converting boxed slices into boxed arrays This mirrors the implementations of reference slices into arrays. # Discussion - [x] Should we use const generics? ([probably not](#61515 (comment))) - [ ] [What's the safety rationale here](#61515 (comment))? - [ ] [Should the errors return the original object](#61515 (comment))? # Remaining - [ ] Implement `Error` - [ ] Create a tracking issue
☀️ Test successful - checks-azure |
This mirrors the implementations of reference slices into arrays.
Discussion
Remaining
Error