[hashmap] Adds `drain`: a way to sneak out the elements while clearing a map. #19946

cgaebel · 2014-12-17T03:33:35Z

It is useful to move all the elements out of a hashmap without deallocating
the underlying buffer. It came up in IRC, and this patch implements it as
drain.

r? @gankro
cc: @frankmcsherry

alexcrichton · 2014-12-17T04:41:06Z

Let's please hold off merging this until rust-lang/rfcs#509 is merged as well (but review is fine!)

pczarn · 2014-12-17T16:28:47Z

src/libstd/collections/hash/table.rs

+            }
+        }
+
+        assert_eq!(self.table.size, 0);


nit: debug_assert_eq!

reem · 2014-12-17T17:11:31Z

cc @aturon this is what we discussed on IRC yesterday.

cgaebel · 2014-12-18T03:19:20Z

cc @thestinger for Vec changes.

RingBuf/Vec .drain has been added if y'all want to review that too!

Gankra · 2014-12-18T03:21:44Z

src/libcollections/ring_buf.rs

-        while self.pop_front().is_some() {}
-        self.head = 0;
-        self.tail = 0;
+        self.drain();


cgaebel · 2014-12-18T03:40:03Z

BinaryHeap is done now, too. Not sure who "owns" that (and ringbuf too, for that matter), but someone should cc them.

cgaebel · 2014-12-18T03:46:46Z

src/libcollections/vec.rs

-    #[inline]
-    /// Drops all items that have not yet been moved and returns the empty vector.
+    /// Deprecated, use .drain() instead
+    #[deprecated = "use .drain() insetad"]


I don't think this deprecation is in the RFC. It should probably be added.

Unsure if this is strictly desirable. MoveItems in theory lets you move the collection to somewhere else by passing around an iterator.

Yeah that's why I'm thinking it should be added to the RFC so people can debate it.

Gankra · 2014-12-18T03:48:26Z

RingBuf is primarily authored by @csherratt. BinaryHeap hasn't really had any heavy work done on it in a while. Pretty simple collection relatively speaking.

sinistersnare · 2014-12-18T03:58:53Z

src/libcollections/binary_heap.rs

@@ -596,6 +605,26 @@ impl<T> DoubleEndedIterator<T> for MoveItems<T> {

 impl<T> ExactSizeIterator<T> for MoveItems<T> {}

+/// An iterator that drains a `BinarHeap`.


s/BinarHeap/BinaryHeap

Gankra · 2014-12-18T04:02:33Z

I take it back, @apasel422 is in charge of BinaryHeap forever now.

ghost · 2014-12-18T05:42:53Z

src/libcollections/ring_buf.rs

 impl<T> ExactSizeIterator<T> for MoveItems<T> {}

+// A draining RingBuf iterator


This should be ///

cgaebel · 2014-12-18T22:59:48Z

This has been rebased, and I believe all review comments have been addressed.

r? @gankro

ghost · 2014-12-18T23:59:33Z

The changes to ringbuf LGTM.

Gankra · 2014-12-19T00:43:45Z

src/libcollections/binary_heap.rs

+    #[inline]
+    #[unstable = "matches collection reform specification, waiting for dust to settle"]
+    pub fn drain<'a>(&'a mut self) -> DrainItems<'a, T> {
+        DrainItems {


Word on high from lord @aturon is that these should be called Drain (as Iterators should be named after their creation method -- Iter, IterMut, IntoIter.

Gankra · 2014-12-19T00:51:47Z

src/libcollections/ring_buf.rs

@@ -443,6 +443,27 @@ impl<T> RingBuf<T> {
    #[unstable = "matches collection reform specification, waiting for dust to settle"]
    pub fn is_empty(&self) -> bool { self.len() == 0 }

+    /// Creates a draining iterator, that is, clears the RingBuf and returns an


Awkward sentence. Either have a period after iterator, or just have "... iterator that clears the..."

Gankra · 2014-12-19T01:54:49Z

src/libstd/collections/hash/map.rs

+    /// Clears the map, returning all key-value pairs as an iterator. Keeps the
+    /// allocated memory for reuse.
+    ///
+    /// # Example


All the other docs in this file say "Example", but in other files it says "Examples" even when there's only one test.

New convention is examples per the rule of @steveklabnik; anything else is out of date.

Even when the english language disagrees? afaik singular forms of nouns should be used when there's only one.

Gankra · 2014-12-19T02:28:00Z

Ok, done my full pass.

cgaebel · 2014-12-19T02:57:48Z

Moi aussi.

Gankra · 2014-12-19T03:14:04Z

r=me with fixed doc + squash

@frankmcsherry

…aring. It is useful to move all the elements out of some collections without deallocating the underlying buffer. It came up in IRC, and this patch implements it as `drain`. This has been discussed as part of RFC 509. r? @gankro cc: @frankmcsherry

cgaebel · 2014-12-19T03:17:45Z

@frankmcsherry

It is useful to move all the elements out of a hashmap without deallocating the underlying buffer. It came up in IRC, and this patch implements it as `drain`. r? @gankro cc: @frankmcsherry

pczarn reviewed Dec 17, 2014
View reviewed changes

src/libstd/collections/hash/table.rs

}

}

assert_eq!(self.table.size, 0);

Copy link

Contributor

pczarn Dec 17, 2014

Choose a reason for hiding this comment

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

nit: debug_assert_eq!

Gankra reviewed Dec 18, 2014
View reviewed changes

src/libcollections/ring_buf.rs

while self.pop_front().is_some() {}

self.head = 0;

self.tail = 0;

self.drain();

Copy link

Contributor

Gankra Dec 18, 2014

Choose a reason for hiding this comment

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

<3

cgaebel reviewed Dec 18, 2014
View reviewed changes

sinistersnare reviewed Dec 18, 2014
View reviewed changes

ghost reviewed Dec 18, 2014
View reviewed changes

src/libcollections/ring_buf.rs

impl<T> ExactSizeIterator<T> for MoveItems<T> {}

// A draining RingBuf iterator

Copy link

ghost Dec 18, 2014

Choose a reason for hiding this comment

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

This should be ///

cgaebel force-pushed the hashmap-drain-iter branch from 0c721de to fcfbefb Compare December 18, 2014 22:58

rust-highfive assigned Gankra Dec 18, 2014

cgaebel force-pushed the hashmap-drain-iter branch from fcfbefb to 9ccd998 Compare December 18, 2014 23:01

Gankra reviewed Dec 19, 2014
View reviewed changes

cgaebel force-pushed the hashmap-drain-iter branch from 9ccd998 to 0925212 Compare December 19, 2014 00:50

Gankra reviewed Dec 19, 2014
View reviewed changes

cgaebel force-pushed the hashmap-drain-iter branch from 992eeef to d57f259 Compare December 19, 2014 03:16

bors merged commit d57f259 into rust-lang:master Dec 21, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hashmap] Adds `drain`: a way to sneak out the elements while clearing a map. #19946

[hashmap] Adds `drain`: a way to sneak out the elements while clearing a map. #19946

cgaebel commented Dec 17, 2014

alexcrichton commented Dec 17, 2014

pczarn Dec 17, 2014

reem commented Dec 17, 2014

cgaebel commented Dec 18, 2014

Gankra Dec 18, 2014

cgaebel commented Dec 18, 2014

cgaebel Dec 18, 2014

Gankra Dec 18, 2014

cgaebel Dec 18, 2014

Gankra commented Dec 18, 2014

sinistersnare Dec 18, 2014

Gankra commented Dec 18, 2014

ghost Dec 18, 2014

cgaebel commented Dec 18, 2014

ghost commented Dec 18, 2014

Gankra Dec 19, 2014

cgaebel Dec 19, 2014

Gankra Dec 19, 2014

Gankra Dec 19, 2014

cgaebel Dec 19, 2014

Gankra Dec 19, 2014

cgaebel Dec 19, 2014

Gankra commented Dec 19, 2014

cgaebel commented Dec 19, 2014

Gankra commented Dec 19, 2014

cgaebel commented Dec 19, 2014

		@@ -596,6 +605,26 @@ impl<T> DoubleEndedIterator<T> for MoveItems<T> {

		impl<T> ExactSizeIterator<T> for MoveItems<T> {}

		/// An iterator that drains a `BinarHeap`.

		impl<T> ExactSizeIterator<T> for MoveItems<T> {}

		// A draining RingBuf iterator

[hashmap] Adds drain: a way to sneak out the elements while clearing a map. #19946

[hashmap] Adds drain: a way to sneak out the elements while clearing a map. #19946

Conversation

cgaebel commented Dec 17, 2014

alexcrichton commented Dec 17, 2014

Choose a reason for hiding this comment

reem commented Dec 17, 2014

cgaebel commented Dec 18, 2014

Choose a reason for hiding this comment

cgaebel commented Dec 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Dec 18, 2014

Choose a reason for hiding this comment

Gankra commented Dec 18, 2014

Choose a reason for hiding this comment

cgaebel commented Dec 18, 2014

ghost commented Dec 18, 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

Gankra commented Dec 19, 2014

cgaebel commented Dec 19, 2014

Gankra commented Dec 19, 2014

cgaebel commented Dec 19, 2014

[hashmap] Adds `drain`: a way to sneak out the elements while clearing a map. #19946

[hashmap] Adds `drain`: a way to sneak out the elements while clearing a map. #19946