Collections Reform Part 2 #509

Merged
merged 2 commits into from Dec 18, 2014

Conversation

Projects
None yet
@Gankro
Contributor

Gankro commented Dec 9, 2014

This RFC shores up the finer details of collections reform. In particular, where the previous RFC
focused on general conventions and patterns, this RFC focuses on specific APIs. It also patches
up any errors that were found during implementation of part 1. Some of these changes
have already been implemented, and simply need to be ratified.

rendered

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Dec 9, 2014

Contributor

CC @aturon @reem @huonw @cmr @pcwalton @nodakai @bfops

One of your desired collections items may have arrived in this RFC.

Contributor

Gankro commented Dec 9, 2014

CC @aturon @reem @huonw @cmr @pcwalton @nodakai @bfops

One of your desired collections items may have arrived in this RFC.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 9, 2014

Contributor

We can do append by just having a Vec::<T>::clear_into_iter::<'a>(&'a mut self) -> MoveItems<T>+'a that sets len to 0. If we want to hack performance we can do it by branching on TypeId, or equivalently on the return value of Any – see http://is.gd/6bznhA (LLVM optimizes this out).

Contributor

arielb1 commented Dec 9, 2014

We can do append by just having a Vec::<T>::clear_into_iter::<'a>(&'a mut self) -> MoveItems<T>+'a that sets len to 0. If we want to hack performance we can do it by branching on TypeId, or equivalently on the return value of Any – see http://is.gd/6bznhA (LLVM optimizes this out).

text/0000-collections-reform-part-2.md
+```
+/// Gets a slice over all the elements in the RingBuf. This may require shifting
+/// all the elements to make this possible.
+pub fn to_slice(&'a self) -> &'a [T]

This comment has been minimized.

@P1start

P1start Dec 9, 2014

Contributor

Wouldn’t this require &mut self to allow shifting the elements inside?

@P1start

P1start Dec 9, 2014

Contributor

Wouldn’t this require &mut self to allow shifting the elements inside?

This comment has been minimized.

@Gankro

Gankro Dec 9, 2014

Contributor

Oops, yes.

@Gankro

Gankro Dec 9, 2014

Contributor

Oops, yes.

@aliblong

This comment has been minimized.

Show comment
Hide comment
@aliblong

aliblong Dec 9, 2014

I prefer reserve_index for VecMap at least. In the example given in the documentation, where a VecMap maps month names to numbers, my thought process would be "How many months are there? 12! So I want to reserve 12 in this VecMap". It would feel clumsy to have to reserve 13, especially given that 0-indexing has taught my brain to think in n-1 with collections.

It's also important to note that whatever design we choose should be mirrored in with_capacity.

aliblong commented Dec 9, 2014

I prefer reserve_index for VecMap at least. In the example given in the documentation, where a VecMap maps month names to numbers, my thought process would be "How many months are there? 12! So I want to reserve 12 in this VecMap". It would feel clumsy to have to reserve 13, especially given that 0-indexing has taught my brain to think in n-1 with collections.

It's also important to note that whatever design we choose should be mirrored in with_capacity.

+
+* Stabilize `front`/`back`/`front_mut`/`back_mut` for peeking on the ends of Deques
+
+* Explicitly specify HashMap's iterators to be non-deterministic between iterations. This would

This comment has been minimized.

@sfackler

sfackler Dec 9, 2014

Member

It seems like this is hacking around the fact that there are types that want to implement ExactSizeIterator but not DoubleEndedIterator. Why don't we clean up that API?

@sfackler

sfackler Dec 9, 2014

Member

It seems like this is hacking around the fact that there are types that want to implement ExactSizeIterator but not DoubleEndedIterator. Why don't we clean up that API?

This comment has been minimized.

@Gankro

Gankro Dec 9, 2014

Contributor

Most things that care about ExactSize also care about DoubleEnded, so I'm not sure if that's totally helpful. I agree that it's a borderline abuse to impl DoubleEnded on an "unordered" sequence, though.

@Gankro

Gankro Dec 9, 2014

Contributor

Most things that care about ExactSize also care about DoubleEnded, so I'm not sure if that's totally helpful. I agree that it's a borderline abuse to impl DoubleEnded on an "unordered" sequence, though.

text/0000-collections-reform-part-2.md
+
+```
+impl Map<K, V> {
+ fn entry<O: ToOwned<K>>(&'a mut self, key: &O) -> Entry<'a, O, V>

This comment has been minimized.

@bstrie

bstrie Dec 9, 2014

Contributor

Should this be entry<'a, O: ToOwned<K>> ?

@bstrie

bstrie Dec 9, 2014

Contributor

Should this be entry<'a, O: ToOwned<K>> ?

This comment has been minimized.

@Gankro

Gankro Dec 9, 2014

Contributor

Yes (actually I think it could all be elided but I'm always paranoid about type lifetime elision)

@Gankro

Gankro Dec 9, 2014

Contributor

Yes (actually I think it could all be elided but I'm always paranoid about type lifetime elision)

This comment has been minimized.

@sfackler

sfackler Dec 9, 2014

Member

Lifetime elision for lifetime parameters of structs is a bad idea, imo, since unlike elision for references there's no way to see that the thing has lifetime restrictions without going in and looking at the type.

@sfackler

sfackler Dec 9, 2014

Member

Lifetime elision for lifetime parameters of structs is a bad idea, imo, since unlike elision for references there's no way to see that the thing has lifetime restrictions without going in and looking at the type.

+* To all collections
+```
+/// Moves all the elements of `other` into `Self`, leaving `other` empty.
+pub fn append(&mut self, other: &mut Self)

This comment has been minimized.

@hexsel

hexsel Dec 9, 2014

maybe really minor, or not relevant since there may not be any read-only collections you can prepend-to only, but "add" may be a more generic method name that doesn't assume which position the data will be added to

@hexsel

hexsel Dec 9, 2014

maybe really minor, or not relevant since there may not be any read-only collections you can prepend-to only, but "add" may be a more generic method name that doesn't assume which position the data will be added to

This comment has been minimized.

@Gankro

Gankro Dec 12, 2014

Contributor

add technically conflicts with the + operator, which e.g. Sets implement for union.

@Gankro

Gankro Dec 12, 2014

Contributor

add technically conflicts with the + operator, which e.g. Sets implement for union.

text/0000-collections-reform-part-2.md
+* To DList, Vec, RingBuf, BitV:
+```
+/// Splits the collection into two at the given index. Useful for similar reasons as `append`.
+pub fn split_at(&mut self, at: uint) -> Self;

This comment has been minimized.

@lifthrasiir

lifthrasiir Dec 10, 2014

Contributor

I think this name is confusing. We already have SlicePrelude::split_at[_mut] which consumes itself and returns two copies of the same type. Shouldn't this be pub fn split_at(self, at: uint) -> (Self, Self); following the precedent?

@lifthrasiir

lifthrasiir Dec 10, 2014

Contributor

I think this name is confusing. We already have SlicePrelude::split_at[_mut] which consumes itself and returns two copies of the same type. Shouldn't this be pub fn split_at(self, at: uint) -> (Self, Self); following the precedent?

This comment has been minimized.

@Gankro

Gankro Dec 10, 2014

Contributor

You're right the name is confusing in the face of the slice method (which I had forgotten about). Still, I think this is the signature we want. Minimizes moves, doesn't require by-value ownership (which of course can be hacked with replace or swap, but I'd rather not if possible).

Perhaps just split would be appropriate?

@Gankro

Gankro Dec 10, 2014

Contributor

You're right the name is confusing in the face of the slice method (which I had forgotten about). Still, I think this is the signature we want. Minimizes moves, doesn't require by-value ownership (which of course can be hacked with replace or swap, but I'd rather not if possible).

Perhaps just split would be appropriate?

This comment has been minimized.

@lifthrasiir

lifthrasiir Dec 11, 2014

Contributor

We also have SlicePrelude::split ;) Maybe this deserves an entirely different name... splice? (This has a precedent in PHP and Perl, except that they can also replace removed elements.)

@lifthrasiir

lifthrasiir Dec 11, 2014

Contributor

We also have SlicePrelude::split ;) Maybe this deserves an entirely different name... splice? (This has a precedent in PHP and Perl, except that they can also replace removed elements.)

This comment has been minimized.

@pczarn

pczarn Dec 11, 2014

I'd name it split_off_at, cut_off_at, or some other synonym_at. (Of course this function's signature makes sense.)

@pczarn

pczarn Dec 11, 2014

I'd name it split_off_at, cut_off_at, or some other synonym_at. (Of course this function's signature makes sense.)

text/0000-collections-reform-part-2.md
+```
+/// Splits the collection into two at the given key. Returns everything after the given key,
+/// including the key.
+pub fn split_at<B: Borrow<K>>(&mut self, at: B) -> Self;

This comment has been minimized.

@lifthrasiir

lifthrasiir Dec 10, 2014

Contributor

Same here. pub fn split_at<B: Borrow<K>>(self, at: B) -> (Self, Self); seems better.

@lifthrasiir

lifthrasiir Dec 10, 2014

Contributor

Same here. pub fn split_at<B: Borrow<K>>(self, at: B) -> (Self, Self); seems better.

text/0000-collections-reform-part-2.md
+}
+
+impl Entry<'a, O: 'a, V:'a> {
+ get(self) -> Result<&'a mut V, VacantEntry<'a, O, V>>

This comment has been minimized.

@lifthrasiir

lifthrasiir Dec 10, 2014

Contributor

Missing fn.

@lifthrasiir

lifthrasiir Dec 10, 2014

Contributor

Missing fn.

@aturon aturon referenced this pull request in rust-lang/rust Dec 11, 2014

Closed

Make `collections::VecMap` generic over its key type #19715

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 11, 2014

Member

Passing thought: should with_capacity on e.g. VecMap be renamed as well?

Member

aturon commented Dec 11, 2014

Passing thought: should with_capacity on e.g. VecMap be renamed as well?

@aliblong

This comment has been minimized.

Show comment
Hide comment
@aliblong

aliblong Dec 11, 2014

Perhaps rename with_capacity to with_reserved_index for VecMap? A bit verbose, but fits nicely with reserve_index.

Perhaps rename with_capacity to with_reserved_index for VecMap? A bit verbose, but fits nicely with reserve_index.

@tbu-

This comment has been minimized.

Show comment
Hide comment
@tbu-

tbu- Dec 11, 2014

Contributor

With the PR rust#19715 I raised the question whether VecMap (and consequently Bitv, TrieMap as @Gankro pointed out) should have a type parameter for the key type. As this lets us add more possible index types in a backward-compatible fashion, I'm very much for it. Could that be added to this RFC?

Contributor

tbu- commented Dec 11, 2014

With the PR rust#19715 I raised the question whether VecMap (and consequently Bitv, TrieMap as @Gankro pointed out) should have a type parameter for the key type. As this lets us add more possible index types in a backward-compatible fashion, I'm very much for it. Could that be added to this RFC?

text/0000-collections-reform-part-2.md
+based around the largest index they have set, and not the number of elements they contain.
+One could instead opt for `reserve_len` or
+`reserve_capacity`, which are effectively the same method, but with an off-by-one. That is,
+`reserve_len(x)` == reserve_index(x - 1)`. `reserve_len` would have the benefit of returning the

This comment has been minimized.

@nodakai

nodakai Dec 12, 2014

Just a nitpicking; markup error here.

`reserve_len(x)` == reserve_index(x - 1)`
@nodakai

nodakai Dec 12, 2014

Just a nitpicking; markup error here.

`reserve_len(x)` == reserve_index(x - 1)`
@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Dec 12, 2014

Contributor

@aturon And I sat down this afternoon and played a little marry/murder/make-out with libcollections. As a result major update:

  • RFC now recommends stabilization/removal paths for all collections
  • RFC now deprecates some Vec and DList methods
  • split_at -> split_off
  • reserve_len wins
  • -> (&[T], &[T]) wins for RingBuf::as_slices
Contributor

Gankro commented Dec 12, 2014

@aturon And I sat down this afternoon and played a little marry/murder/make-out with libcollections. As a result major update:

  • RFC now recommends stabilization/removal paths for all collections
  • RFC now deprecates some Vec and DList methods
  • split_at -> split_off
  • reserve_len wins
  • -> (&[T], &[T]) wins for RingBuf::as_slices
+///
+/// Calls either `grow()` or `truncate()` depending on whether `new_len`
+/// is larger than the current value of `len()` or not.
+pub fn resize(&mut self, new_len: uint, value: T) where T: Clone

This comment has been minimized.

@alexcrichton

alexcrichton Dec 12, 2014

Member

Out of curiosity, could you explain a little more the rationale for including this convenience method? I'm not sure I've personally ever needed it, but I may just be in a different line of work!

@alexcrichton

alexcrichton Dec 12, 2014

Member

Out of curiosity, could you explain a little more the rationale for including this convenience method? I'm not sure I've personally ever needed it, but I may just be in a different line of work!

This comment has been minimized.

@Gankro

Gankro Dec 12, 2014

Contributor

This is something a few people has asked for. I don't have a strong desire/motivation for it, but I thought I would give it a fair try out in this RFC.

Asked for in rust-lang/rust#19104 (comment)

so maybe @nodakai can give a better justification.

@Gankro

Gankro Dec 12, 2014

Contributor

This is something a few people has asked for. I don't have a strong desire/motivation for it, but I thought I would give it a fair try out in this RFC.

Asked for in rust-lang/rust#19104 (comment)

so maybe @nodakai can give a better justification.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 12, 2014

Member

Ah ok, I may be forgetting, but do we have a number of other convenience methods like this on collections? I thought we had pared it back to a fairly bare-bones sense.

@alexcrichton

alexcrichton Dec 12, 2014

Member

Ah ok, I may be forgetting, but do we have a number of other convenience methods like this on collections? I thought we had pared it back to a fairly bare-bones sense.

This comment has been minimized.

@Valloric

Valloric Dec 12, 2014

If one wants to ensure that a vector is exactly 10 items long, this is the best method to call since it will work right even if the vector is currently 9 or 11 items long.

IMO grow and truncate are the convenience methods, not resize.

@Valloric

Valloric Dec 12, 2014

If one wants to ensure that a vector is exactly 10 items long, this is the best method to call since it will work right even if the vector is currently 9 or 11 items long.

IMO grow and truncate are the convenience methods, not resize.

This comment has been minimized.

@Valloric

Valloric Dec 12, 2014

It would be worth looking at what the C++ std::vector provides: http://en.cppreference.com/w/cpp/container/vector

(Spoiler: It's only resize.)

@Valloric

Valloric Dec 12, 2014

It would be worth looking at what the C++ std::vector provides: http://en.cppreference.com/w/cpp/container/vector

(Spoiler: It's only resize.)

This comment has been minimized.

@Gankro

Gankro Dec 12, 2014

Contributor

@Valloric But growing and shrinking really are very different operations. resize necessarily must branch on whether to grow or shrink, whereas grow and truncate don't.

@Gankro

Gankro Dec 12, 2014

Contributor

@Valloric But growing and shrinking really are very different operations. resize necessarily must branch on whether to grow or shrink, whereas grow and truncate don't.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 12, 2014

Member

@Valloric it would be difficult to remove truncate as you can't always manifest a T to call resize to truncate. C++ looks like it leverages overloading as well to provide some more functionality which we may not be able to match exactly.

@alexcrichton

alexcrichton Dec 12, 2014

Member

@Valloric it would be difficult to remove truncate as you can't always manifest a T to call resize to truncate. C++ looks like it leverages overloading as well to provide some more functionality which we may not be able to match exactly.

text/0000-collections-reform-part-2.md
+}
+
+impl Deref<V> for OccupiedEntry<'a, O, V>
+impl DerefMut<V> for OccupiedEntry<'a, O, V>

This comment has been minimized.

@alexcrichton

alexcrichton Dec 12, 2014

Member

In the past we've tended to avoid Deref implementations on structures that otherwise have a number of inherent methods. For example the fact that MutexGuard used to have a cond field was seen as somewhat unpopular because it sometimes a field would appear out of thin air.

While definitely an ergonomic improvement, do you think that these entries are used often enough to warrant the departure from this semi-convention?

@alexcrichton

alexcrichton Dec 12, 2014

Member

In the past we've tended to avoid Deref implementations on structures that otherwise have a number of inherent methods. For example the fact that MutexGuard used to have a cond field was seen as somewhat unpopular because it sometimes a field would appear out of thin air.

While definitely an ergonomic improvement, do you think that these entries are used often enough to warrant the departure from this semi-convention?

This comment has been minimized.

@Gankro

Gankro Dec 12, 2014

Contributor

I'm not sure this convention is well followed in the present. Rc and Vec being two notable examples. This change in particular was suggested by @cmr.

It sort of makes sense, really. An entry is really just a very smart pointer to an element.

@Gankro

Gankro Dec 12, 2014

Contributor

I'm not sure this convention is well followed in the present. Rc and Vec being two notable examples. This change in particular was suggested by @cmr.

It sort of makes sense, really. An entry is really just a very smart pointer to an element.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 12, 2014

Member

Vec is a bit of an outlier because we know precisely which type we're implementing Deref for, which is just a slice. Rc, on the other hand, is a good point! Looking back at its API there are more methods than I originally thought.

However, in general it's a pretty dangerous operation to implement Deref because it means we'll never be able to backwards-compatible add a method to an OccupiedEntry (or Rc for that matter). In the case of Vec we can continue to add methods because we know exactly what we're deref'ing to, and we can know if they clash or not.

We don't currently have strong conventions around when to or not implement Deref, so I guess I should rephrase my concern/question: How sure are you that the API of OccupiedEntry will not need to grow in the future?

@alexcrichton

alexcrichton Dec 12, 2014

Member

Vec is a bit of an outlier because we know precisely which type we're implementing Deref for, which is just a slice. Rc, on the other hand, is a good point! Looking back at its API there are more methods than I originally thought.

However, in general it's a pretty dangerous operation to implement Deref because it means we'll never be able to backwards-compatible add a method to an OccupiedEntry (or Rc for that matter). In the case of Vec we can continue to add methods because we know exactly what we're deref'ing to, and we can know if they clash or not.

We don't currently have strong conventions around when to or not implement Deref, so I guess I should rephrase my concern/question: How sure are you that the API of OccupiedEntry will not need to grow in the future?

This comment has been minimized.

@Gankro

Gankro Dec 12, 2014

Contributor

Ah interesting, I hadn't though of it in that terms. There is the possibility of eventually adding methods to ask about/for the key contained in the entry.

@Gankro

Gankro Dec 12, 2014

Contributor

Ah interesting, I hadn't though of it in that terms. There is the possibility of eventually adding methods to ask about/for the key contained in the entry.

This comment has been minimized.

@mahkoh

mahkoh Dec 12, 2014

Contributor

However, in general it's a pretty dangerous operation to implement Deref because it means we'll never be able to backwards-compatible add a method to an OccupiedEntry (or Rc for that matter).

Adding public methods is always backwards incompatible because of extension traits.

@mahkoh

mahkoh Dec 12, 2014

Contributor

However, in general it's a pretty dangerous operation to implement Deref because it means we'll never be able to backwards-compatible add a method to an OccupiedEntry (or Rc for that matter).

Adding public methods is always backwards incompatible because of extension traits.

This comment has been minimized.

@aturon

aturon Dec 13, 2014

Member

@alexcrichton

However, in general it's a pretty dangerous operation to implement Deref because it means we'll never be able to backwards-compatible add a method to an OccupiedEntry (or Rc for that matter).

True, but you can always add extension traits. But I think you make a compelling argument here; we shouldn't implement Deref at this point, and we can always add it later if we believe the rest of the API is ready to freeze or it's just to painful to live without.

@aturon

aturon Dec 13, 2014

Member

@alexcrichton

However, in general it's a pretty dangerous operation to implement Deref because it means we'll never be able to backwards-compatible add a method to an OccupiedEntry (or Rc for that matter).

True, but you can always add extension traits. But I think you make a compelling argument here; we shouldn't implement Deref at this point, and we can always add it later if we believe the rest of the API is ready to freeze or it's just to painful to live without.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 12, 2014

Member

This looks awesome @Gankro, nice work!

Member

alexcrichton commented Dec 12, 2014

This looks awesome @Gankro, nice work!

@aturon aturon referenced this pull request in rust-lang/rust Dec 13, 2014

Closed

Stabilization metabug: 1.0-alpha #19260

140 of 142 tasks complete
@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Dec 13, 2014

Contributor

@thestinger: for the sake of transparency, I feel obligated to notify you that the current draft of this RFC is recommending the removal of Tree*, Trie*, LruCache, bitflags!, and EnumSet from the standard library. I believe you were opposed to this in the past.

Contributor

Gankro commented Dec 13, 2014

@thestinger: for the sake of transparency, I feel obligated to notify you that the current draft of this RFC is recommending the removal of Tree*, Trie*, LruCache, bitflags!, and EnumSet from the standard library. I believe you were opposed to this in the past.

+* Unstable: Bitv, BitvSet, VecMap
+* Move to [collect-rs](https://github.com/Gankro/collect-rs/) for incubation:
+EnumSet, bitflags!, LruCache, TreeMap, TreeSet, TrieMap, TrieSet
+

This comment has been minimized.

@bluss

bluss Dec 15, 2014

Do you think DList is ready? It's unclear if it is near its final form.

@bluss

bluss Dec 15, 2014

Do you think DList is ready? It's unclear if it is near its final form.

This comment has been minimized.

@pczarn

pczarn Dec 15, 2014

And why do you think bitflags isn't ready? I agree about other collections.

@pczarn

pczarn Dec 15, 2014

And why do you think bitflags isn't ready? I agree about other collections.

This comment has been minimized.

@Gankro

Gankro Dec 15, 2014

Contributor

Collections Reform 1 already proposed cutting out a lot of DList's methods, so it's going to have a pretty minimal/uncontroversial interface.

As for bitflags, I'll leave that to @aturon

@Gankro

Gankro Dec 15, 2014

Contributor

Collections Reform 1 already proposed cutting out a lot of DList's methods, so it's going to have a pretty minimal/uncontroversial interface.

As for bitflags, I'll leave that to @aturon

+* Stable: Vec, RingBuf, HashMap, HashSet, BTreeMap, BTreeSet, DList, BinaryHeap
+* Unstable: Bitv, BitvSet, VecMap
+* Move to [collect-rs](https://github.com/Gankro/collect-rs/) for incubation:
+EnumSet, bitflags!, LruCache, TreeMap, TreeSet, TrieMap, TrieSet

This comment has been minimized.

@cgaebel

cgaebel Dec 16, 2014

Contributor

Can we rename BTreeMap -> TreeMap, BTreeSet -> TreeSet? The b is an "implementation detail".

@cgaebel

cgaebel Dec 16, 2014

Contributor

Can we rename BTreeMap -> TreeMap, BTreeSet -> TreeSet? The b is an "implementation detail".

This comment has been minimized.

@Gankro

Gankro Dec 16, 2014

Contributor

I don't feel it is. For one we expose a with_b constructor (though this will remain experimental). Otherwise I think it's relevant information to someone who actually cares about that sort of thing. Otherwise people will just see SomethingSomethingTreeMap and use it anyway.

And TreeMap may one day return.

@Gankro

Gankro Dec 16, 2014

Contributor

I don't feel it is. For one we expose a with_b constructor (though this will remain experimental). Otherwise I think it's relevant information to someone who actually cares about that sort of thing. Otherwise people will just see SomethingSomethingTreeMap and use it anyway.

And TreeMap may one day return.

+```
+/// Gets two slices that cover the whole range of the RingBuf.
+/// The second one may be empty. Otherwise, it continues *after* the first.
+pub fn as_slices(&'a self) -> (&'a [T], &'a [T])

This comment has been minimized.

@cgaebel

cgaebel Dec 16, 2014

Contributor

Needs an as_mut_slices version, too.

@cgaebel

cgaebel Dec 16, 2014

Contributor

Needs an as_mut_slices version, too.

cgaebel added a commit to cgaebel/rust that referenced this pull request Dec 16, 2014

Add RingBuf::as_slices as per collections reform v2.
See: rust-lang/rfcs#509

Not sure if this is allowed to land before the RFC. Either way,
it's here for review.

r? @Gankro
cc: @bfops
Merge pull request #1 from cgaebel/drain
Add drain, and link to WIP implementations.
@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Dec 17, 2014

Contributor

Added a drain iterator for moving out of an &mut Collection without wasting the allocation.

Contributor

Gankro commented Dec 17, 2014

Added a drain iterator for moving out of an &mut Collection without wasting the allocation.

@michaelsproul

This comment has been minimized.

Show comment
Hide comment
@michaelsproul

michaelsproul Dec 17, 2014

+1 for removing TrieMap/TrieSet from the standard distribution. Hopefully this will pave the way for out-of-tree (pardon the pun) Trie implementations. I've drawn up a design on paper for a generic Patricia Trie myself, but I am yet to implement it...

+1 for removing TrieMap/TrieSet from the standard distribution. Hopefully this will pave the way for out-of-tree (pardon the pun) Trie implementations. I've drawn up a design on paper for a generic Patricia Trie myself, but I am yet to implement it...

@pczarn

This comment has been minimized.

Show comment
Hide comment
@pczarn

pczarn Dec 18, 2014

+1 for all points. Also, hardcoding uint keys for Trie* made it nearly useless. @michaelsproul I have a (broken, not yet functional) Patricia Trie implementation.

pczarn commented Dec 18, 2014

+1 for all points. Also, hardcoding uint keys for Trie* made it nearly useless. @michaelsproul I have a (broken, not yet functional) Patricia Trie implementation.

@michaelsproul

This comment has been minimized.

Show comment
Hide comment
@michaelsproul

michaelsproul Dec 18, 2014

@pczarn: Sounds like we should collaborate! The only code I've written so far is for a "nibble vector", which I was planning to use to store 4-bit key fragments (for 16-child branching).

https://github.com/michaelsproul/rust-nibble-vec

@pczarn: Sounds like we should collaborate! The only code I've written so far is for a "nibble vector", which I was planning to use to store 4-bit key fragments (for 16-child branching).

https://github.com/michaelsproul/rust-nibble-vec

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 18, 2014

Member

Thanks for the RFC, @Gankro! After discussion with the community, the core team has decided to move forward with this set of API improvements. New APIs here (like drain) should land as #[unstable], and we'll make a final decision about committing to them during stabilization, after we've gotten more feedback in the wild.

While most of the collections being removed are already available in an external crate, we believe that bitflags should go in a standalone crate as it's somewhat of an outlier. @bjz is likely the right person to do this.

Member

aturon commented Dec 18, 2014

Thanks for the RFC, @Gankro! After discussion with the community, the core team has decided to move forward with this set of API improvements. New APIs here (like drain) should land as #[unstable], and we'll make a final decision about committing to them during stabilization, after we've gotten more feedback in the wild.

While most of the collections being removed are already available in an external crate, we believe that bitflags should go in a standalone crate as it's somewhat of an outlier. @bjz is likely the right person to do this.

@aturon aturon merged commit 480eaa7 into rust-lang:master Dec 18, 2014

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Dec 18, 2014

Contributor

Yes I avoided stealing bitflags for this reason.

Great to land this!

Contributor

Gankro commented Dec 18, 2014

Yes I avoided stealing bitflags for this reason.

Great to land this!

@aturon

This comment has been minimized.

Show comment
Hide comment
Member

aturon commented Dec 18, 2014

bors added a commit to rust-lang/rust that referenced this pull request Dec 18, 2014

auto merge of #19955 : Gankro/rust/kill-all-code, r=aturon
EnumSet lives on in libcollections so that librustc can still use it. This adds a direct dependency on libcollections to librustc and libserialize.

Should not be merged until rust-lang/rfcs#509 is accepted.

All of these collections have already been moved to collect-rs where they will ideally be maintained and experimented with, or will be replaced by something better: https://github.com/Gankro/collect-rs/

[breaking-change]

r? @aturon @alexcrichton

bors added a commit to rust-lang/rust that referenced this pull request Dec 19, 2014

auto merge of #19955 : Gankro/rust/kill-all-code, r=aturon
EnumSet lives on in libcollections so that librustc can still use it. This adds a direct dependency on libcollections to librustc and libserialize.

Should not be merged until rust-lang/rfcs#509 is accepted.

All of these collections have already been moved to collect-rs where they will ideally be maintained and experimented with, or will be replaced by something better: https://github.com/Gankro/collect-rs/

[breaking-change]

r? @aturon @alexcrichton

cgaebel added a commit to cgaebel/rust that referenced this pull request Dec 19, 2014

Add RingBuf::as_slices as per collections reform v2.
See: rust-lang/rfcs#509

Not sure if this is allowed to land before the RFC. Either way,
it's here for review.

r? @Gankro
cc: @bfops

@csouth3 csouth3 referenced this pull request in rust-lang/rust Dec 20, 2014

Merged

Misc Stabilization for collections #20053

bors added a commit to rust-lang/rust that referenced this pull request Dec 20, 2014

auto merge of #19903 : cgaebel/rust/ringbuf-as-slice, r=Gankro
See: rust-lang/rfcs#509

Not sure if this is allowed to land before the RFC. Either way,
it's here for review.

r? @Gankro
cc: @bfops

aturon added a commit to aturon/rust that referenced this pull request Dec 20, 2014

Second pass stabilization: vec
This commit takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_buf`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

[breaking-change]

@aturon aturon referenced this pull request in rust-lang/rust Dec 20, 2014

Closed

Second pass stabilization: slice and vec #20061

bors added a commit to rust-lang/rust that referenced this pull request Dec 20, 2014

auto merge of #19903 : cgaebel/rust/ringbuf-as-slice, r=Gankro
See: rust-lang/rfcs#509

Not sure if this is allowed to land before the RFC. Either way,
it's here for review.

r? @Gankro
cc: @bfops

bors added a commit to rust-lang/rust that referenced this pull request Dec 22, 2014

auto merge of #20053 : csouth3/rust/misc-stab, r=alexcrichton
This pull request:

*Renames `BinaryHeap::top` to `BinaryHeap::peek`
*Stabilizes `front/back/front_mut/back_mut` in `DList` and `RingBuf`
*Stabilizes `swap` in `RingBuf`

in accordance with rust-lang/rfcs#509.

Note that this PR does not address `Bitv::{get,set}` or HashMap's iterators, nor does it move `std::vec` to `std::collections::vec`, all of which still need to be done.

Because of the method renaming, this is a [breaking-change].

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 22, 2014

rollup merge of #20053: csouth3/misc-stab
This pull request:

*Renames `BinaryHeap::top` to `BinaryHeap::peek`
*Stabilizes `front/back/front_mut/back_mut` in `DList` and `RingBuf`
*Stabilizes `swap` in `RingBuf`

in accordance with rust-lang/rfcs#509.

Note that this PR does not address `Bitv::{get,set}` or HashMap's iterators, nor does it move `std::vec` to `std::collections::vec`, all of which still need to be done.

Because of the method renaming, this is a [breaking-change].

@csouth3 csouth3 referenced this pull request in rust-lang/rust Dec 24, 2014

Merged

Deprecate `DList::ListInsertion` #20185

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Dec 24, 2014

What did you intend w.r.t ExactSizeIterator and DoubleEndedIterator? I'm against implementing traits just because it's possible, with the hash maps and sets it doesn't seem useful at all. Maybe ExactSizeIterator on its own if it is untangled from DEI, but even there I don't really know any real use.

bluss commented Dec 24, 2014

What did you intend w.r.t ExactSizeIterator and DoubleEndedIterator? I'm against implementing traits just because it's possible, with the hash maps and sets it doesn't seem useful at all. Maybe ExactSizeIterator on its own if it is untangled from DEI, but even there I don't really know any real use.

bors added a commit to rust-lang/rust that referenced this pull request Dec 25, 2014

auto merge of #20143 : csouth3/rust/vecmap-reserve, r=Gankro
Implement `reserve_len` and `reserve_len_exact` for `VecMap` in accordance with rust-lang/rfcs#509.

bors added a commit to rust-lang/rust that referenced this pull request Dec 27, 2014

auto merge of #20143 : csouth3/rust/vecmap-reserve, r=Gankro
Implement `reserve_len` and `reserve_len_exact` for `VecMap` in accordance with rust-lang/rfcs#509.

bors added a commit to rust-lang/rust that referenced this pull request Dec 28, 2014

auto merge of #20185 : csouth3/rust/dlist-deprecate, r=alexcrichton
This PR deprecates the `DList::ListInsertion` trait, in accordance with rust-lang/rfcs#509.  The functions which were previously part of the ListInsertion impl for `DList::IterMut` have been moved to be inherent methods on the iterator itself, and appropriate doctests have been added.

michaelwoerister added a commit to michaelwoerister/rust that referenced this pull request Dec 29, 2014

Add RingBuf::as_slices as per collections reform v2.
See: rust-lang/rfcs#509

Not sure if this is allowed to land before the RFC. Either way,
it's here for review.

r? @Gankro
cc: @bfops

aturon added a commit to aturon/rust that referenced this pull request Dec 30, 2014

Second pass stabilization: vec
This commit takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

[breaking-change]

aturon added a commit to aturon/rust that referenced this pull request Dec 30, 2014

Second pass stabilization: vec
This commit takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

[breaking-change]

aturon added a commit to aturon/rust that referenced this pull request Dec 30, 2014

Second pass stabilization: vec
This commit takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

[breaking-change]

bors added a commit to rust-lang/rust that referenced this pull request Dec 30, 2014

auto merge of #20061 : aturon/rust/stab-2-vec-slice, r=alexcrichton
# Second pass stabilization: vec

This PR takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

# Second pass stabilization: slice

This PR takes a second pass through the `slice` module to
stabilize its API. The changes are as follows:

**Stable**:

* `as_mut_slice`
* `as_ptr`, `as_mut_ptr`
* `back`, `back_mut` (was: `last`)
* `binary_search_by` (was: `binary_search`)
* `binary_search` (was: `binary_search_elem`)
* `chunks`, `chunks_mut`
* `contains`
* `ends_with`
* `front`, `front_mut` (was: `head`)
* `get_unchecked`, `get_unchecked_mut` (was: `unsafe_get`)
* `get`
* `is_empty`
* `iter`, `iter_mut`
* `len`
* `permutations`
* `reverse`
* `sort_by`
* `sort`
* `split_at`, `split_at_mut`
* `split_mut`, `splitn_mut`, `rsplitn_mut`
* `split`, `splitn`, `rsplitn`
* `starts_with`
* `swap`
* `to_vec`
* `windows`

**Deprecated**:

* `head`, `head_mut`, `last`, `last_mut` (renamed as above)
* `unsafe_get`, `unsafe_mut` (renamed as above)
* `binary_search_elem` (renamed as above)
* `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.
* `BinarySearchResult`, deprecated in favor of `Result<uint, uint>`

# Unify concat and concat_vec

We've long had traits `StrVector` and `VectorVector` providing
`concat`/`connect` and `concat_vec`/`connect_vec` respectively. The
reason for the distinction is that coherence rules did not used to be
robust enough to allow impls on e.g. `Vec<String>` versus `Vec<&[T]>`.

This PR consolidates the traits into a single `SliceConcatExt` trait
provided by `slice` and the prelude (where it replaces `StrVector`,
which is removed.)

[breaking-change]

r? @alexcrichton

aturon added a commit to aturon/rust that referenced this pull request Dec 30, 2014

Second pass stabilization: vec
This commit takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

[breaking-change]

aturon added a commit to aturon/rust that referenced this pull request Dec 30, 2014

Second pass stabilization: vec
This commit takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

[breaking-change]

bors added a commit to rust-lang/rust that referenced this pull request Dec 31, 2014

auto merge of #20061 : aturon/rust/stab-2-vec-slice, r=alexcrichton
# Second pass stabilization: vec

This PR takes a second pass through the `vec` module to
stabilize its API. The changes are as follows:

**Stable**:

* `dedup`
* `from_raw_parts`
* `insert`
* `into_iter`
* `is_empty`
* `remove`
* `reserve_exact`
* `reserve`
* `retain`
* `swap_remove`
* `truncate`

**Deprecated**:

* `from_fn`, `from_elem`, `grow_fn` and `grow`, all deprecated in
  favor of iterators. See rust-lang/rfcs#509

* `partition`, `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.

* `unzip`, deprecated in favor of a new, more general iterator
  consumer called `unzip`.

A few remaining methods are left at experimental status.

# Second pass stabilization: slice

This PR takes a second pass through the `slice` module to
stabilize its API. The changes are as follows:

**Stable**:

* `as_mut_slice`
* `as_ptr`, `as_mut_ptr`
* `binary_search_by` (was: `binary_search`)
* `binary_search` (was: `binary_search_elem`)
* `chunks`, `chunks_mut`
* `contains`
* `ends_with`
* `first`, `first_mut` (was: `head`)
* `get_unchecked`, `get_unchecked_mut` (was: `unsafe_get`)
* `get`
* `is_empty`
* `iter`, `iter_mut`
* `len`
* `permutations`
* `reverse`
* `sort_by`
* `sort`
* `split_at`, `split_at_mut`
* `split_mut`, `splitn_mut`, `rsplitn_mut`
* `split`, `splitn`, `rsplitn`
* `starts_with`
* `swap`
* `to_vec`
* `windows`

**Deprecated**:

* `head`, `head_mut` (renamed as above)
* `unsafe_get`, `unsafe_mut` (renamed as above)
* `binary_search_elem` (renamed as above)
* `partitioned`, deprecated in favor of a new, more
  general iterator consumer called `partition`.
* `BinarySearchResult`, deprecated in favor of `Result<uint, uint>`

# Unify concat and concat_vec

We've long had traits `StrVector` and `VectorVector` providing
`concat`/`connect` and `concat_vec`/`connect_vec` respectively. The
reason for the distinction is that coherence rules did not used to be
robust enough to allow impls on e.g. `Vec<String>` versus `Vec<&[T]>`.

This PR consolidates the traits into a single `SliceConcatExt` trait
provided by `slice` and the prelude (where it replaces `StrVector`,
which is removed.)

[breaking-change]

r? @alexcrichton
@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Jan 2, 2015

Is it possible to amend the RFC to keep .merge() on DList? It seems absurd to add new "allocation-free" method to DList while removing others like .merge(). I'm mostly confused by the cluelessness of the deprecation message "Use other methods on IterMut"; it would be better to be more honest and tell the user there is no replacement.

bluss commented Jan 2, 2015

Is it possible to amend the RFC to keep .merge() on DList? It seems absurd to add new "allocation-free" method to DList while removing others like .merge(). I'm mostly confused by the cluelessness of the deprecation message "Use other methods on IterMut"; it would be better to be more honest and tell the user there is no replacement.

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Jan 2, 2015

Contributor

I assure you merge was never allocation free; it just used IterMut.

Contributor

Gankro commented Jan 2, 2015

I assure you merge was never allocation free; it just used IterMut.

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Jan 2, 2015

Contributor

Oh no wait, it totally did! I misread the source.

Contributor

Gankro commented Jan 2, 2015

Oh no wait, it totally did! I misread the source.

@drbawb drbawb referenced this pull request in reem/rust-typemap Jan 6, 2015

Closed

Does not build on latest nightly w/ RFC 509 #21

bors added a commit to rust-lang/rust that referenced this pull request Jan 11, 2015

auto merge of #20406 : TimDumol/rust/dlist-append-split-off, r=Gankro
Implements the `append()` and `split_off()` methods proposed by the collections reform part 2 RFC.

RFC: rust-lang/rfcs#509
Tracking issue: #19986

@erickt erickt referenced this pull request in rust-lang/rust Jan 17, 2015

Closed

Rename string::push_str to string::concat #20857

@Gankro Gankro referenced this pull request in rust-lang/rust Jan 19, 2015

Merged

add append to vec with tests and benchmarks #21330

@sandeep-datta

This comment has been minimized.

Show comment
Hide comment
@sandeep-datta

sandeep-datta Mar 30, 2015

I was told to leave a comment here instead of opening a new issue:
Rename string::push_str to string::concat #20857

Please consider renaming string::push_str() to string::concat(). Push_X() in the context of a collection means X will be added to the container as a single entity. Hence push_str() is meaningless for strings since a string can only contain characters (char != [char]).

If it is too late to rename then consider adding a method called concat() anyway.

Also consider renaming Vec::push_all() to Vec::concat() so that the name is consistent with String::concat().

I was told to leave a comment here instead of opening a new issue:
Rename string::push_str to string::concat #20857

Please consider renaming string::push_str() to string::concat(). Push_X() in the context of a collection means X will be added to the container as a single entity. Hence push_str() is meaningless for strings since a string can only contain characters (char != [char]).

If it is too late to rename then consider adding a method called concat() anyway.

Also consider renaming Vec::push_all() to Vec::concat() so that the name is consistent with String::concat().

@aturon aturon referenced this pull request in rust-lang/rust Aug 12, 2015

Closed

Tracking issue for `drain` stabilization #27711

@Gankro Gankro referenced this pull request in rust-lang/rust Oct 23, 2015

Closed

Tracking issue for Vec::resize #27790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment