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

Implement RFC 839 #25989

Merged
merged 1 commit into from
Jun 8, 2015
Merged

Implement RFC 839 #25989

merged 1 commit into from
Jun 8, 2015

Conversation

jooert
Copy link
Contributor

@jooert jooert commented Jun 3, 2015

I had to use impl<'a, V: Copy> Extend<(usize, &'a V)> for VecMap<V> instead of impl<'a, V: Copy> Extend<(&'a usize, &'a V)> for VecMap<V> as that's what is needed for doing

let mut a = VecMap::new();
let b = VecMap::new();
b.insert(1, "foo");

a.extend(&b)

I can squash the commits after review.

r? @gankro

@@ -760,3 +760,10 @@ impl<T: Ord> Extend<T> for BinaryHeap<T> {
}
}
}

#[unstable(feature = "extend_ref", reason = "recently added")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the way stability works these instability annotations unfortunately won't be read and considered. In this case that's ok, though, but could you change these to #[stable(feature = "extend_ref", since = "1.2.0")]?

@Gankra
Copy link
Contributor

Gankra commented Jun 3, 2015

🎊 r=me with @alexcrichton's nits addressed

@jooert
Copy link
Contributor Author

jooert commented Jun 7, 2015

Sorry for the delay! I modified the stability markers and removed #![feature(extend_ref)] from libcollectionstest/lib.rs.

@Gankra
Copy link
Contributor

Gankra commented Jun 8, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2015

📌 Commit e2dd771 has been approved by Gankro

@bors
Copy link
Contributor

bors commented Jun 8, 2015

⌛ Testing commit e2dd771 with merge 0246d8d...

@bors
Copy link
Contributor

bors commented Jun 8, 2015

💔 Test failed - auto-linux-64-x-android-t

@Gankra
Copy link
Contributor

Gankra commented Jun 8, 2015

/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/librustdoc/html/render.rs:2291:25: 2291:36 error: the type of this value must be known in this context
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/librustdoc/html/render.rs:2291             let did = i.trait_did().unwrap();
                                                                                                                                   ^~~~~~~~~~~
note: in expansion of for loop expansion
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/librustdoc/html/render.rs:2290:9: 2293:10 note: expansion site
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/librustdoc/html/render.rs:2299:27: 2299:40 error: the type of this value must be known in this context
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/librustdoc/html/render.rs:2299                 let did = i.trait_did().unwrap();
                                                                                                                                     ^~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/librustdoc/html/render.rs:2298:13: 2301:14 note: expansion site
error: aborting due to 2 previous errors
make: *** [x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/stamp.rustdoc] Error 101
make: *** Waiting for unfinished jobs....

@jooert
Copy link
Contributor Author

jooert commented Jun 8, 2015

Oh, sorry, I fixed it.
This seems to be one of those cases where inference fails due to the new implementations of Extend.

@Gankra
Copy link
Contributor

Gankra commented Jun 8, 2015

@bors r+

No worries!

@bors
Copy link
Contributor

bors commented Jun 8, 2015

📌 Commit b36ed7d has been approved by Gankro

bors added a commit that referenced this pull request Jun 8, 2015
I had to use `impl<'a, V: Copy> Extend<(usize, &'a V)> for VecMap<V>` instead of `impl<'a, V: Copy> Extend<(&'a usize, &'a V)> for VecMap<V>` as that's what is needed for doing

```rust
let mut a = VecMap::new();
let b = VecMap::new();
b.insert(1, "foo");

a.extend(&b)
```

I can squash the commits after review.

r? @gankro
@bors
Copy link
Contributor

bors commented Jun 8, 2015

⌛ Testing commit b36ed7d with merge 4a397dd...

@bors bors merged commit b36ed7d into rust-lang:master Jun 8, 2015
@bluss
Copy link
Member

bluss commented Jun 8, 2015

Such inference failures are interesting & important to note since this will be a minor breaking change in that case. (Which is permitted). Thanks for implementing this!!

@jooert jooert deleted the implement_rfc839 branch June 8, 2015 19:01
@bluss
Copy link
Member

bluss commented Jun 9, 2015

@brson Do we need to track this? This caused a very minor change to how Extend-using functions infer. In this case seen when using Iterator::unzip.

@brson brson added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jun 9, 2015
@brson
Copy link
Contributor

brson commented Jun 9, 2015

@bluss If it's a regression, yeah, I suppose so. Also nominating for beta for the same reason.

@Gankra
Copy link
Contributor

Gankra commented Jun 9, 2015

@bluss I didn't spell out that in the RFC because I guess it seemed obvious: every generalization of an API can break someone's code in inference.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 10, 2015
@alexcrichton
Copy link
Member

We discussed this in triage today and decided to not backport this, we're going to have minor regressions like this and we're probably not going to aggressively backport them, they'll make their way into the stable channel eventually.

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 16, 2015
bors added a commit that referenced this pull request Sep 1, 2015
It appears that these impls were left out of #25989 by mistake.

r? @alexcrichton

I'm not sure what the stability markers for these should be.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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.

6 participants