Implement Clone for Box<[T]> where T: Clone #26934

reem · 2015-07-10T09:53:20Z

rust-highfive · 2015-07-10T09:53:32Z

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2015-07-10T15:40:06Z

src/liballoc/boxed.rs

+        unsafe { mem::transmute(Slice {
+             data: data,
+             len: self.len()
+        }) }


Stylistically I think we've been moving to "one unsafe block" instead of a few scattered ones, so I think it's fine to just go ahead and wrap this entire method in an unsafe block.

alexcrichton · 2015-07-10T15:43:30Z

Implementation looks good to me! Just some stylistic nits here and there.

cc @gankro

Gankra · 2015-07-10T15:53:10Z

Uhhhhh

Why not just (&self[..]).to_owned().into_boxed_slice()

Gankra · 2015-07-10T15:54:17Z

Ah just read the issue. Sucks.

Gankra · 2015-07-10T15:59:31Z

So funny story I'm actually in the middle of abstracting basically all of Vec's allocation logic into a seperate type: https://github.com/rust-lang/rust/compare/master...Gankro:raw-vec?expand=1

It would be trivial to add into_box/from_box methods that would make this simple. That seems like a cleaner way forward.

bors · 2015-07-10T16:27:20Z

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

Gankra · 2015-07-10T20:48:03Z

src/liballoc/boxed.rs

+impl<T: Clone> Clone for Box<[T]> {
+    fn clone(&self) -> Self {
+        let mut alloc = unsafe {
+            heap::allocate(mem::size_of::<T>() * self.len(),


You can't do this unconditionally; you need to special case mem::size_of::<T> = 0 to not allocate (use alloc::heap::EMPTY).

reem · 2015-07-10T23:02:30Z

@gankro shall we let this sit until RawVec lands then? It already needs a rebase because of CString changes.

EDIT: I am open to doing the rebase and landing now too, up to you.

Gankra · 2015-07-10T23:05:42Z

Easier to land RawVec first. I hope to post a WIP PR tonight; looks like mostly writing tests now.

alexcrichton · 2015-07-15T16:38:10Z

r? @gankro

Gankra · 2015-07-15T21:59:36Z

This is blocked on #26955 regardless

On Wed, Jul 15, 2015 at 10:15 AM, Tamir Duberstein <[email protected]

wrote:

@alexcrichton https://github.com/alexcrichton merge conflict

—
Reply to this email directly or view it on GitHub
#26934 (comment).

Gankra · 2015-07-18T03:43:45Z

@reem it landed. Should be trivial to implement this on top, now.

Gankra · 2015-07-27T01:42:25Z

@reem ping

reem · 2015-07-27T05:14:06Z

@gankro @alexcrichton pushed an updated version based on RawVec.

Gankra · 2015-07-27T05:20:25Z

src/liballoc/boxed.rs

+        let raw = ptr::read(&self.data);
+        mem::forget(self);
+        raw.into_box()
+    }


Any reason to factor this out? If it's only going to be used in one place, I favour to keep it inline. Especially since it's unsafe.

I just felt philosophically that it was part of BoxBuilder's interface and it was strange for the Clone impl to be messing around like this with the internals of the type when it's custom built for that use case.

Honestly now that you mention it, I'd even hoist the type into the function so no one else can use it.

Gankra · 2015-07-27T05:27:20Z

r=me with nits

Gankra · 2015-07-27T16:00:46Z

src/liballoc/boxed.rs

@@ -511,3 +512,53 @@ impl<'a,A,R> FnOnce<A> for Box<FnBox<A,Output=R>+Send+'a> {
 }

 impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<Box<U>> for Box<T> {}
+
+impl<T: Clone> Clone for Box<[T]> {


Oh I guess we add stability markers to these now

#[stable(feature = "boxed_slice_clone", since = "1.3.0")]

impls are auto-stable anyway so there is no point, iirc

We like to track when impls like this were added in case we do start tracking impl stability one day.

reem · 2015-07-28T04:25:59Z

Added a stability marker and hoisted the helper type into the implementation.

reem · 2015-07-28T04:49:50Z

Travis appears to have failed but I don't see why...

Gankra · 2015-07-28T06:11:11Z

Pretty failed out. Right now the build powers through to the next thing anyway.

check: feature sanity

error: misformed stability attribute

line 519 of /home/travis/build/rust-lang/rust/src/liballoc/boxed.rs:

#[stable(feature = "box-slice-clone", since = "1.3.0")]

Pretty sure features have to be valid identifiers (underscores).

Closes rust-lang#25097

reem · 2015-07-28T08:43:49Z

Updated with box_slice_clone.

Gankra · 2015-07-28T14:35:34Z

@bors r+

bors · 2015-07-28T14:35:34Z

📌 Commit e244230 has been approved by Gankro

bors · 2015-07-28T16:53:26Z

⌛ Testing commit e244230 with merge 07e2ce9...

bors · 2015-07-28T17:58:17Z

💔 Test failed - auto-mac-64-nopt-t

alexcrichton · 2015-07-28T18:38:12Z

@bors: retry

On Tue, Jul 28, 2015 at 10:58 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-mac-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/5910

—
Reply to this email directly or view it on GitHub
#26934 (comment).

bluss · 2015-07-28T19:35:30Z

This PR should maybe say [breaking-change] in the commit log.

Breaking case is that calling .to_owned() on Box<[T]> where T: Clone today produces Vec<T>, I think it would produce Box<[T]> with this PR. I also examined .clone(), but that is invalid on current Box<[T]> (does not produce &[T] as I had feared).

This is a minor breaking change.

bors · 2015-07-28T19:36:27Z

⌛ Testing commit e244230 with merge ba9224f...

Closes #25097

bors · 2015-07-28T21:14:27Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

Gankra · 2015-07-28T21:26:29Z

Thanks!

reem · 2015-07-29T01:09:09Z

Woo!

rust-highfive assigned alexcrichton Jul 10, 2015

alexcrichton reviewed Jul 10, 2015
View reviewed changes

Gankra reviewed Jul 10, 2015
View reviewed changes

rust-highfive assigned Gankra and unassigned alexcrichton Jul 15, 2015

reem force-pushed the boxed-slice-clone branch from b77f4de to 0c15fca Compare July 27, 2015 05:12

Gankra reviewed Jul 27, 2015
View reviewed changes

reem force-pushed the boxed-slice-clone branch from 0c15fca to 94a02d2 Compare July 28, 2015 04:24

Implement Clone for Box<[T]> where T: Clone

e244230

Closes rust-lang#25097

reem force-pushed the boxed-slice-clone branch from 94a02d2 to e244230 Compare July 28, 2015 08:43

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 28, 2015

bors added a commit that referenced this pull request Jul 28, 2015

Auto merge of #26934 - reem:boxed-slice-clone, r=Gankro

ba9224f

Closes #25097

bors merged commit e244230 into rust-lang:master Jul 28, 2015

reem deleted the boxed-slice-clone branch July 29, 2015 01:09

solson mentioned this pull request Aug 3, 2015

Fix Clone for Box<[T]> issue title. rust-lang/this-week-in-rust#100

Merged

Implement Clone for Box<[T]> where T: Clone #26934

Implement Clone for Box<[T]> where T: Clone #26934

Conversation

reem commented Jul 10, 2015

rust-highfive commented Jul 10, 2015

Choose a reason for hiding this comment

alexcrichton commented Jul 10, 2015

Gankra commented Jul 10, 2015

Gankra commented Jul 10, 2015

Gankra commented Jul 10, 2015

bors commented Jul 10, 2015

Choose a reason for hiding this comment

reem commented Jul 10, 2015

Gankra commented Jul 10, 2015

alexcrichton commented Jul 15, 2015

Gankra commented Jul 15, 2015

Gankra commented Jul 18, 2015

Gankra commented Jul 27, 2015

reem commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reem commented Jul 28, 2015

reem commented Jul 28, 2015

Gankra commented Jul 28, 2015

reem commented Jul 28, 2015

Gankra commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

alexcrichton commented Jul 28, 2015

bluss commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

Gankra commented Jul 28, 2015

reem commented Jul 29, 2015