stabilize more of collections #20356

Gankra · 2014-12-31T00:18:20Z

This stabilizes most of libcollections, carefully avoiding sections of API which are being managed in other PRs. APIs which are not stable are marked explicitly unstable with a reason.

Deprecates:

DList
- rotate_forward
- rotate_backward
- prepend
- insert_when
- insert_ordered
- merge
VecMap
- update
- update_with_key
Renames and newtypes the Bitv and BitvSet iterators to match conventions.
Removes the Copy impl from DList's Iter.

as such this is a

[breaking-change]

rust-highfive · 2014-12-31T00:18:26Z

r? @huonw

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

Gankra · 2014-12-31T00:18:38Z

r? @aturon

Gankra · 2014-12-31T00:20:12Z

@csouth3 if you're still on a quest to Fix All The Iterators, Bitv/BitvSet still need renaming/newtyping.

alexcrichton · 2014-12-31T00:33:33Z

src/libcollections/bit.rs

@@ -578,7 +579,7 @@ impl Bitv {
    /// assert_eq!(bv.iter().filter(|x| *x).count(), 7);
    /// ```
    #[inline]
-    #[unstable = "matches collection reform specification, waiting for dust to settle"]
+    #[stable]
    pub fn iter<'a>(&'a self) -> Bits<'a> {


I've been taking advantage of elision wherever possible when stabilizing APIs as it generally makes the signatures look a little prettier (just something to keep in mind).

This may also not want to be stable just yet until Bits has been renamed.

The collection maintainers are a bit... paranoid... about elision when it comes to types. I can change them all, if you'd like.

Also I've seen some argue that it doesn't necessarily help with clarity in this case, because it's not "clear" that the returned type borrows. You have to know that it's eliding a lifetime.

I've generally been removing them for strings (and I think @aturon has as well for vectors), so we should probably be consistent at the very least. I understand low level code wanting to avoid elision, but I'm not sure I buy the "without elision it's more clear" case because that's largely the purpose of elision (to move the lifetimes out of the way of what's going on).

For all these higher-level apis for collections I would personally elide as much as possible, but I'm curious what @aturon thinks

This seems mostly like a rustdoc bug; e.g. rustdoc could/should show all signatures elided by default, with an option to display elided lifetimes, entirely independent of whether or not there are lifetimes in the source.

I do understand the worry about confusion when a type "silently" takes a
lifetime parameter, but this is why we have ensured it's very easy to
determine whether a type does: it's explicit in the definition. (Other
options would allow it to be implicit even there, but that seemed truly
problematic.) My feeling is that, just as you learn that & takes a
lifetime, you come to learn this about other types in play. It's similar to
knowing whether a type is Copy or not -- it's a very fundamental aspect of
the type. So I personally have no problem eliding in these cases.

As @huonw notes, though, what shows up in rustdoc should probably be more
uniform/independent of these concerns.

On Tue, Dec 30, 2014 at 6:34 PM, Alex Crichton notifications@github.com
wrote:

In src/libcollections/bit.rs
#20356 (diff):

@@ -578,7 +579,7 @@ impl Bitv {
/// assert_eq!(bv.iter().filter(|x| *x).count(), 7);
/// ```
#[inline]

#[unstable = "matches collection reform specification, waiting for dust to settle"]

#[stable]
pub fn iter<'a>(&'a self) -> Bits<'a> {

I've generally been removing them for strings (and I think @aturon
https://github.com/aturon has as well for vectors), so we should
probably be consistent at the very least. I understand low level code
wanting to avoid elision, but I'm not sure I buy the "without elision it's
more clear" case because that's largely the purpose of elision (to move the
lifetimes out of the way of what's going on).

For all these higher-level apis for collections I would personally elide
as much as possible, but I'm curious what @aturon
https://github.com/aturon thinks

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/20356/files#r22373132.

csouth3 · 2014-12-31T01:24:27Z

@gankro I'd be glad to

but I'm away on a trip so I can't submit the patch until this Thursday afternoon. If that timeline would block other important progress too long then by all means go ahead and get it done sooner. Otherwise I'll get right on it as soon as I can!

Gankra · 2014-12-31T01:50:38Z

Ack, I've run into the tangled-ness of the Bitv types. They both want an Iter type. Might need to do some trix to get the exports right.

csouth3 · 2014-12-31T01:52:56Z

Oh that's right, I hadn't thought of that...sounds somewhat messy.

Gankra · 2014-12-31T03:33:22Z

@alexcrichton I think I've addressed all your issues. There weren't too many methods I could elide since many used the lifetime variable in the body, or had "more exotic" lifetime usage.

Gankra · 2014-12-31T05:10:37Z

I've squashed all the updates into a second commit which I can fully squash when you've reviewed.

alexcrichton · 2014-12-31T16:19:53Z

src/libcollections/bit.rs

-    pub fn iter<'a>(&'a self) -> BitPositions<'a> {
-        BitPositions {set: self, next_idx: 0u}
+    #[stable]
+    pub fn iter(&self) -> SetIter {


Hm, maybe this should be #[unstable] to see if we can figure out what to do about this iterator?

The public API should show it yields bitv_set::Iter, as this is the only public name it is available under.

alexcrichton · 2014-12-31T16:24:52Z

Thanks looks fantastic @gankro, thanks!

I'm basically ready to r+, but I'd like to give @aturon a chance to look this over as well to double check me.

Gankra · 2014-12-31T16:39:16Z

nits addressed

aturon · 2014-12-31T18:09:16Z

src/libcollections/bit.rs

-    pub fn iter<'a>(&'a self) -> BitPositions<'a> {
-        BitPositions {set: self, next_idx: 0u}
+    #[stable]
+    pub fn iter(&self) -> bitv_set::Iter {


How will this return type show up in rustdoc?

aturon · 2014-12-31T18:15:32Z

OK, I've looked this over and it looks fantastic. Nice work by the whole community rolling collections reform out! I'm so excited to see this land. :)

Gankra · 2014-12-31T18:41:46Z

squashed

aturon · 2014-12-31T23:18:24Z

Needs a rebase.

This stabilizes most of libcollections, carefully avoiding sections of API which are being managed in other PRs. APIs which are not stable are marked explicitly unstable with a reason. Deprecates: * DList * rotate_forward * rotate_backward * prepend * insert_when * insert_ordered * merge * VecMap * update * update_with_key * Renames and newtypes the Bitv and BitvSet iterators to match conventions. * Removes the Copy impl from DList's Iter. as such this is a [breaking-change]

alexcrichton · 2015-01-02T16:14:36Z

I pushed this manually to master as it passed all platforms but one where a test timed out (a known failure case today).

bluss · 2015-01-02T16:39:28Z

src/libcollections/dlist.rs

-    ///
-    /// This operation should compute in O(max(N, M)) time.
+    /// Deprecated: Use custom methods on IterMut.
+    #[deprecated = "Use custom methods on IterMut"]
    pub fn merge<F>(&mut self, mut other: DList<T>, mut f: F) where F: FnMut(&T, &T) -> bool {


If the user has to implement merge themselves (using other methods), then that will require deallocating and allocating a lot of list nodes, while merge merges two DLists without a single allocation. It should stay.

It's a shame to remove the methods that make DList uniquely useful.

Yes you're right; I misread the source of it. That said, can you provide a usecase for using DList like this?

Also if you're interested in really complex DList manipulation, consider checking out my experimental DList with a full Cursor API, which I think is the more correct solution to this kind of complex manipulation problem.

I'd really love to get more feedback from DList power-users!

…manieu Remove surplus prepend LinkedList fn This nightly library feature provides a function on `LinkedList<T>` that is identical to `fn append` with a reversed order of arguments. Observe this diff against the `fn append` doctest: ```diff +#![feature(linked_list_prepend)] fn main() { use std::collections::LinkedList; let mut list1 = LinkedList::new(); list1.push_back('a'); let mut list2 = LinkedList::new(); list2.push_back('b'); list2.push_back('c'); - list1.append(&mut list2); + list2.prepend(&mut list1); - let mut iter = list1.iter(); + let mut iter = list2.iter(); assert_eq!(iter.next(), Some(&'a')); assert_eq!(iter.next(), Some(&'b')); assert_eq!(iter.next(), Some(&'c')); assert!(iter.next().is_none()); - assert!(list2.is_empty()); + assert!(list1.is_empty()); } ``` As this has received no obvious request to stabilize it, nor does it have a tracking issue, and was left on nightly and the consensus seems to have been to deprecate it in this pre-1.0 PR in 2014, rust-lang#20356, I propose simply removing it.

rust-highfive assigned huonw Dec 31, 2014

rust-highfive assigned aturon and unassigned huonw Dec 31, 2014

aturon mentioned this pull request Dec 31, 2014

Stabilization metabug: 1.0-alpha #19260

Closed

alexcrichton reviewed Dec 31, 2014
View reviewed changes

Gankra force-pushed the stab branch from d26cc4b to 765b9a4 Compare December 31, 2014 01:35

Gankra force-pushed the stab branch from f5fe428 to 63fd687 Compare December 31, 2014 05:08

alexcrichton reviewed Dec 31, 2014
View reviewed changes

Gankra force-pushed the stab branch from 9b8ed0b to ba04fb8 Compare December 31, 2014 16:39

aturon reviewed Dec 31, 2014
View reviewed changes

Gankra force-pushed the stab branch from ba04fb8 to ae25604 Compare December 31, 2014 18:41

stabilize more of collections

8dbaa71

Gankra force-pushed the stab branch from ae25604 to 8dbaa71 Compare December 31, 2014 23:54

alexcrichton merged commit 8dbaa71 into rust-lang:master Jan 2, 2015

bluss reviewed Jan 2, 2015
View reviewed changes

workingjubilee mentioned this pull request May 21, 2021

Remove surplus prepend LinkedList fn #85571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stabilize more of collections #20356

stabilize more of collections #20356

Gankra commented Dec 31, 2014

rust-highfive commented Dec 31, 2014

Gankra commented Dec 31, 2014

Gankra commented Dec 31, 2014

alexcrichton Dec 31, 2014

alexcrichton Dec 31, 2014

Gankra Dec 31, 2014

Gankra Dec 31, 2014

alexcrichton Dec 31, 2014

huonw Dec 31, 2014

aturon Dec 31, 2014

csouth3 commented Dec 31, 2014

Gankra commented Dec 31, 2014

csouth3 commented Dec 31, 2014

Gankra commented Dec 31, 2014

Gankra commented Dec 31, 2014

alexcrichton Dec 31, 2014

Gankra Dec 31, 2014

alexcrichton commented Dec 31, 2014

Gankra commented Dec 31, 2014

aturon Dec 31, 2014

aturon commented Dec 31, 2014

Gankra commented Dec 31, 2014

aturon commented Dec 31, 2014

alexcrichton commented Jan 2, 2015

bluss Jan 2, 2015

Gankra Jan 2, 2015

Gankra Jan 2, 2015

stabilize more of collections #20356

stabilize more of collections #20356

Conversation

Gankra commented Dec 31, 2014

rust-highfive commented Dec 31, 2014

Gankra commented Dec 31, 2014

Gankra commented Dec 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csouth3 commented Dec 31, 2014

Gankra commented Dec 31, 2014

csouth3 commented Dec 31, 2014

Gankra commented Dec 31, 2014

Gankra commented Dec 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Dec 31, 2014

Gankra commented Dec 31, 2014

Choose a reason for hiding this comment

aturon commented Dec 31, 2014

Gankra commented Dec 31, 2014

aturon commented Dec 31, 2014

alexcrichton commented Jan 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment