Implement TryFrom<Box<[T]>> for Box<[T; N]> #47245

nvzqz · 2018-01-07T00:45:55Z

These changes serve to provide the same benefits as PR #44764 except for boxes, which refer to owned arrays allocated in the heap.

Should these changes be extended to other types like Arc<[T]> and Rc<[T]>?

r? @sfackler

This commit serves to provide the same benefits as PR rust-lang#44764 except for boxes that refer to owned arrays allocated in the heap.

kennytm

Is commit 781574787ae169ba4fe87a71730a5a2b1638761b a mistake? Why is there a merge commit in this PR?

kennytm · 2018-01-07T08:16:41Z

src/liballoc/boxed.rs

+}
+
+macro_rules! array_impls {
+    ($($N:expr)+) => {


You forgot to add $(...)+ around the item.

[00:05:12] Compiling alloc v0.0.0 (file:///checkout/src/liballoc) [00:05:12] error: variable 'N' is still repeating at this depth [00:05:12] --> liballoc/boxed.rs:665:47 [00:05:12] | [00:05:12] 665 | impl<T> TryFrom<Box<[T]>> for Box<[T; $N]> { [00:05:12] | ^^ [00:05:12] [00:05:12] error: Could not compile `alloc`.

The functions are very short and there may be relatively unreasonable calling overhead if they're not inlined.

sfackler · 2018-01-07T21:00:56Z

src/liballoc/boxed.rs

+                fn try_from(slice: Box<[T]>) -> Result<Box<[T; $N]>, TryFromSliceError<T>> {
+                    if slice.len() == $N {
+                        let ptr = Box::into_raw(slice) as *mut [T; $N];
+                        unsafe { Ok(Box::from_raw(ptr)) }


@alexcrichton is this a valid operation?

AFAIK yeah this should be safe, the destructor for Box<T> should deallocate with the size of T which in this case would be mem::size_of::<T>() * N

sfackler · 2018-01-10T06:11:43Z

Could you give some examples where you'd use these conversions?

nvzqz · 2018-01-10T07:23:41Z

Here a Redditor asked about how to go about making a heap-allocated table with a static size. I recommended using Vec and then converting that into a boxed slice. Because the size is known at compile-time, there's not much reason to keep the dynamic length.

Having const generics as well as a safe conversion interface such as this one would solve his problem without the need for unsafe. So while this conversion as-is wouldn't solve his problem, it in a way potentially solves half of it.

In my experience, LLVM seems to optimize further with arrays (&[T; N]) than just slices (&[T]). For people who want to make their code as fast as possible, they may want to reach for fixed-size arrays.

We currently have &[T] to &[T; N] conversions, I think it also makes sense to have the corresponding box conversions. Without this change, getting &[T; N] from Box<[T]> would require checking the length each time.

sfackler · 2018-01-15T04:21:44Z

That post was asking about [T; 262144], though. Are these conversions worth it before const generics?

carols10cents · 2018-01-22T21:42:47Z

Ping @nvzqz, looks like @sfackler has a further question for you?

nvzqz · 2018-01-22T21:55:42Z

@sfackler to me it makes sense to have these conversions when we already have &[T] to &[T; N] conversions. If these conversions don't get added now, I'll be submitting another similar PR once const generics are in nightly.

Currently, unsafe code is needed by libraries to make a statically sized box. If a statically-size slice is needed, multiple checks must be made for whether the box is the right size if authors choose to avoid unsafe.

sfackler · 2018-01-22T22:00:55Z

Currently, unsafe code is needed by libraries to make a statically sized box.

??

let b: Box<[u64; 100]> = Box::new([0; 100]);

nvzqz · 2018-01-22T23:42:54Z

I should have elaborated. Your example is possible with trivial/primitive types that implement Copy. However, one may want to do something like this:

struct Entry {
    /* fields */
}

const NUM_ENTRIES: usize = 32;

lazy_static! {
    static ref TABLE: Box<[Entry; NUM_ENTRIES]> = {
        let mut table = Vec::<Entry>::with_capacity(NUM_ENTRIES);
        for i in 0..NUM_ENTRIES {
            table.push(Entry { /* ... */ });
        }
        table.into_boxed_slice().try_into().unwrap()
    };
}

By using the Vec to Box<[T]> conversion as well as these changes, this can be done without unsafe.

sfackler · 2018-01-23T00:05:20Z

But how big is NUM_ENTRIES in practice? I'd expect it to almost always be larger than 32.

nvzqz · 2018-01-23T00:48:56Z

Well, generally, perhaps. Unless this is something that's polled, one can't really give a proper number. Alas, it is likely to be greater than 32.

kennytm · 2018-01-31T10:00:08Z

Hi @sfackler @nvzqz, ping from triage.

Given the proper use case of this usually require N >> 32, I guess we should postpone this PR until const generics is implemented?

pietroalbini · 2018-02-05T10:41:08Z

@sfackler @nvzqz ping from triage! What's the status of this PR?

shepmaster · 2018-02-09T22:36:20Z

Since we haven't heard from @sfackler in a few weeks, I'm randomly reassigning from the @rust-lang/libs team...

r? @dtolnay

dtolnay · 2018-02-13T18:46:26Z

I would be inclined to accept this by analogy with #44764. If we decide that array stuff is not worth doing until const generics are available, let's roll back the &[T]⟶&[T; N] conversion impls.

@rfcbot fcp merge

rfcbot · 2018-02-13T18:46:27Z

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

dtolnay · 2018-02-16T17:37:06Z

TryFrom is unstable so I am going to unilaterally accept these and we can consider more closely as a team when stabilizing TryFrom. Thanks @nvzqz!

@rfcbot fcp cancel

rfcbot · 2018-02-16T17:37:07Z

@dtolnay proposal cancelled.

dtolnay · 2018-02-16T17:42:56Z

src/liballoc/boxed.rs

+/// array fails.
+#[unstable(feature = "try_from", issue = "33417")]
+#[derive(Clone)]
+pub struct TryFromSliceError<T>(Box<[T]>);


Please add a brief example involving TryFromSliceError. See FromUtf8Error for examples of examples.

dtolnay · 2018-02-16T17:44:48Z

src/liballoc/boxed.rs

+    /// be made.
+    #[unstable(feature = "try_from", issue = "33417")]
+    #[inline]
+    pub fn into_boxed_slice(self) -> Box<[T]> {


Please add an example showing a case in which this method is useful. The point of the example wouldn't be to show how to call this method (we can assume readers know how to call methods when they get to browsing TryFrom documentation) but to show why someone would want to call this method.

dtolnay · 2018-02-16T17:46:27Z

src/liballoc/boxed.rs

+                type Error = TryFromSliceError<T>;
+
+                #[inline]
+                fn try_from(slice: Box<[T]>) -> Result<Box<[T; $N]>, TryFromSliceError<T>> {


Please add a test so that we don't break this implementation later. Something similar to the test in #44764 should work.

pietroalbini · 2018-02-26T16:41:14Z

Hi from the release team @nvzqz, there are a few comments from @dtolnay that needs to be addressed, can you fix them so this PR can be merged?

pietroalbini · 2018-03-12T10:23:26Z

Thank you so much for this PR @nvzqz! Unfortunately, we haven't heard from you for more than two weeks, so I'm closing this PR to keep the queue tidy.

Please reopen it if you have time to address @dtolnay comments in the near future!

Implement TryFrom for array box types

22c581b

This commit serves to provide the same benefits as PR rust-lang#44764 except for boxes that refer to owned arrays allocated in the heap.

rust-highfive assigned sfackler Jan 7, 2018

kennytm reviewed Jan 7, 2018

View reviewed changes

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 7, 2018

nvzqz force-pushed the try-from-slice branch from 7815747 to 22c581b Compare January 7, 2018 19:55

nvzqz added 2 commits January 7, 2018 14:58

Add missing repetition for array_impls! macro item

b25334a

Inline all TryFrom<[T]> for [T; N] instances

8cb0b3e

The functions are very short and there may be relatively unreasonable calling overhead if they're not inlined.

sfackler reviewed Jan 7, 2018

View reviewed changes

nvzqz added 5 commits January 7, 2018 17:26

Add try_from feature to liballoc

45d6839

Remove Copy derive from boxed::TryFromSliceError

da9a570

Add stability attribute to TryFromSliceError nodes

909e35f

Import 'boxed' module to use TryFromSliceError

aba11fd

Manually implement Debug for TryFromSliceError

544fb2a

alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2018

sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 10, 2018

carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2018

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2018

rust-highfive assigned dtolnay and unassigned sfackler Feb 9, 2018

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 13, 2018

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2018

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 16, 2018

dtolnay reviewed Feb 16, 2018

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 16, 2018

pietroalbini closed this Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TryFrom<Box<[T]>> for Box<[T; N]> #47245

Implement TryFrom<Box<[T]>> for Box<[T; N]> #47245

nvzqz commented Jan 7, 2018

kennytm left a comment

kennytm Jan 7, 2018

sfackler Jan 7, 2018

alexcrichton Jan 8, 2018

sfackler commented Jan 10, 2018

nvzqz commented Jan 10, 2018 •

edited

Loading

sfackler commented Jan 15, 2018

carols10cents commented Jan 22, 2018

nvzqz commented Jan 22, 2018

sfackler commented Jan 22, 2018

nvzqz commented Jan 22, 2018

sfackler commented Jan 23, 2018

nvzqz commented Jan 23, 2018

kennytm commented Jan 31, 2018

pietroalbini commented Feb 5, 2018

shepmaster commented Feb 9, 2018

dtolnay commented Feb 13, 2018

rfcbot commented Feb 13, 2018 •

edited by Kimundi

Loading

dtolnay commented Feb 16, 2018

rfcbot commented Feb 16, 2018

dtolnay Feb 16, 2018

dtolnay Feb 16, 2018

dtolnay Feb 16, 2018

pietroalbini commented Feb 26, 2018

pietroalbini commented Mar 12, 2018

Implement TryFrom<Box<[T]>> for Box<[T; N]> #47245

Implement TryFrom<Box<[T]>> for Box<[T; N]> #47245

Conversation

nvzqz commented Jan 7, 2018

kennytm left a comment

Choose a reason for hiding this comment

kennytm Jan 7, 2018

Choose a reason for hiding this comment

sfackler Jan 7, 2018

Choose a reason for hiding this comment

alexcrichton Jan 8, 2018

Choose a reason for hiding this comment

sfackler commented Jan 10, 2018

nvzqz commented Jan 10, 2018 • edited Loading

sfackler commented Jan 15, 2018

carols10cents commented Jan 22, 2018

nvzqz commented Jan 22, 2018

sfackler commented Jan 22, 2018

nvzqz commented Jan 22, 2018

sfackler commented Jan 23, 2018

nvzqz commented Jan 23, 2018

kennytm commented Jan 31, 2018

pietroalbini commented Feb 5, 2018

shepmaster commented Feb 9, 2018

dtolnay commented Feb 13, 2018

rfcbot commented Feb 13, 2018 • edited by Kimundi Loading

dtolnay commented Feb 16, 2018

rfcbot commented Feb 16, 2018

dtolnay Feb 16, 2018

Choose a reason for hiding this comment

dtolnay Feb 16, 2018

Choose a reason for hiding this comment

dtolnay Feb 16, 2018

Choose a reason for hiding this comment

pietroalbini commented Feb 26, 2018

pietroalbini commented Mar 12, 2018

nvzqz commented Jan 10, 2018 •

edited

Loading

rfcbot commented Feb 13, 2018 •

edited by Kimundi

Loading