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

Specialize Vec::extend to Vec::extend_from_slice #37094

Merged
merged 1 commit into from Oct 15, 2016

Conversation

Projects
None yet
6 participants
@fhartwig
Copy link
Contributor

fhartwig commented Oct 11, 2016

I tried using the existing SpecExtend as a helper trait for this, but the instances would always conflict with the instances higher up in the file, so I created a new helper trait.

Benchmarking extend vs extend_from_slice with an slice of 1000 u64s gives the following results:

before:

running 2 tests
test tests::bench_extend_from_slice ... bench:         166 ns/iter (+/- 78)
test tests::bench_extend_trait      ... bench:       1,187 ns/iter (+/- 697)


after:
running 2 tests
test tests::bench_extend_from_slice ... bench:         149 ns/iter (+/- 87)
test tests::bench_extend_trait      ... bench:         138 ns/iter (+/- 70)
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 11, 2016

r? @alexcrichton

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

@fhartwig

This comment has been minimized.

Copy link
Contributor

fhartwig commented Oct 11, 2016

I'm not really sure what to do with the docstring on extend_from_slice. Saying that extend specialises to the same thing seems premature, considering that specialisation isn't stable yet. But leaving it probably also isn't ideal.

@apasel422

This comment has been minimized.

Copy link
Member

apasel422 commented Oct 12, 2016

Out of curiosity, what were the conflicts with SpecExtend?

@fhartwig

This comment has been minimized.

Copy link
Contributor

fhartwig commented Oct 12, 2016

@apasel422 As far as I remember, I tried to do
impl <'a, T: 'a + Copy, I: IntoIterator<Item=&'a T>> SpecExtend<I> for Vec<T>
and
impl<'a, T: Copy> SpecExtend<&'a[T]> for Vec<T>
and both of these would get a conflicting implementation error pointing to the impl<I: IntoIterator> SpecExtend<I> for Vec<I::Item> that comes before. I might be getting this wrong though, but I can look it up tonight.

EDIT:
Here are the errors that I got when I tried implementing SpecExtend. Looking at them now, they kind of make sense to me:

error[E0119]: conflicting implementations of trait `SpecExtend<_>` for type `vec::Vec<_>`:
    --> src/libcollections/vec.rs:1597:1
     |
1538 | impl<I, T> SpecExtend<I> for Vec<T> where I: IntoIterator<Item=T> {
     | - first implementation here
...
1597 | impl <'a, T: 'a + Copy, I: IntoIterator<Item=&'a T>> SpecExtend<I> for Vec<T> {
     | ^ conflicting implementation for `vec::Vec<_>`

error[E0119]: conflicting implementations of trait `SpecExtend<&[_]>` for type `vec::Vec<_>`:
    --> src/libcollections/vec.rs:1603:1
     |
1538 | impl<I, T> SpecExtend<I> for Vec<T> where I: IntoIterator<Item=T> {
     | - first implementation here
...
1603 | impl<'a, T: Copy> SpecExtend<&'a[T]> for Vec<T> {
     | ^ conflicting implementation for `vec::Vec<_>`

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 13, 2016

@bors: r+

Thanks!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 13, 2016

📌 Commit 63ee8d0 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 13, 2016

⌛️ Testing commit 63ee8d0 with merge 5059dfe...

bors added a commit that referenced this pull request Oct 13, 2016

Auto merge of #37094 - fhartwig:spec-extend-from-slice, r=alexcrichton
Specialize Vec::extend to Vec::extend_from_slice

I tried using the existing `SpecExtend` as a helper trait for this, but the instances would always conflict with the instances higher up in the file, so I created a new helper trait.

Benchmarking `extend` vs `extend_from_slice` with an slice of 1000 `u64`s gives the following results:

```
before:

running 2 tests
test tests::bench_extend_from_slice ... bench:         166 ns/iter (+/- 78)
test tests::bench_extend_trait      ... bench:       1,187 ns/iter (+/- 697)

after:
running 2 tests
test tests::bench_extend_from_slice ... bench:         149 ns/iter (+/- 87)
test tests::bench_extend_trait      ... bench:         138 ns/iter (+/- 70)
```
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 13, 2016

💔 Test failed - auto-linux-cross-opt

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Oct 13, 2016

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 13, 2016

⛄️ The build was interrupted to prioritize another pull request.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 15, 2016

⌛️ Testing commit 63ee8d0 with merge 030bc49...

bors added a commit that referenced this pull request Oct 15, 2016

Auto merge of #37094 - fhartwig:spec-extend-from-slice, r=alexcrichton
Specialize Vec::extend to Vec::extend_from_slice

I tried using the existing `SpecExtend` as a helper trait for this, but the instances would always conflict with the instances higher up in the file, so I created a new helper trait.

Benchmarking `extend` vs `extend_from_slice` with an slice of 1000 `u64`s gives the following results:

```
before:

running 2 tests
test tests::bench_extend_from_slice ... bench:         166 ns/iter (+/- 78)
test tests::bench_extend_trait      ... bench:       1,187 ns/iter (+/- 697)

after:
running 2 tests
test tests::bench_extend_from_slice ... bench:         149 ns/iter (+/- 87)
test tests::bench_extend_trait      ... bench:         138 ns/iter (+/- 70)
```

@bors bors merged commit 63ee8d0 into rust-lang:master Oct 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

mbrubeck added a commit to mbrubeck/website that referenced this pull request Jan 21, 2017

Use `extend` instead of `extend_from_slice`
These are identical since rust-lang/rust#37094 landed, and
`extend_from_slice` will be deprecated in the future.

mbrubeck added a commit to mbrubeck/tokio-line that referenced this pull request Jan 21, 2017

Use `extend` instead of `extend_from_slice`
These are identical since rust-lang/rust#37094 landed, and
extend_from_slice will be deprecated in the future.

See also tokio-rs/website#49.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment