Implement drain over a range for VecDeque #27723

mystor · 2015-08-12T18:01:53Z

This is a WIP PR for my implementation of drain over the VecDeque data structure supporting ranges. It brings the VecDeque drain implementation in line with Vec's.

Tests haven't been written for the new function yet.

rust-highfive · 2015-08-12T18:02:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gankro (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bluss · 2015-08-14T20:52:19Z

RFC: rust-lang/rfcs/pull/1257

bors · 2015-08-16T06:56:56Z

☔ The latest upstream changes (presumably #27818) made this pull request unmergeable. Please resolve the merge conflicts.

Gankra · 2015-08-20T18:25:09Z

src/libcollections/vec_deque.rs

-        self.inner.pop_front()
+        self.iter.next().map(|elt|
+            unsafe {
+                ptr::read(elt as *const _)


I'm pretty sure these as's shouldn't be neccesary

copied from my mistake of doing this in vec's drain

(back then it might've been necessary fwiw -- full &mut to *const coercion is relatively new)

This can be fixed, just drop the cast.

bluss · 2015-08-31T16:06:26Z

I was given the review from gankro

mystor · 2015-09-22T18:00:04Z

Ping @bluss :)

bluss · 2015-09-22T19:25:52Z

I'm reading! Pen & paper ready. I'm sorry that I didn't get to it sooner.

bluss · 2015-09-22T21:37:53Z

src/libcollections/vec_deque.rs

+        // "forget" about the values after the start of the drain until after
+        // the drain is complete and the Drain destructor is run.
+        self.head = drain_tail;
+


From reading experience, this code (which indices are stored where) would be easier to understand with a comment.

The deque's elements are parted into three segments, separated by the indices tail, drain_tail, drain_head, head. tail, drain_tail are stored in self, and drain_head, head are stored in the Drain's after_ fields.

Maybe a comment that explains this or at least explains the after_* names.

bluss · 2015-09-22T21:47:22Z

Very nice work! Everything looks very solid. Love the exhaustive testing.

As a general point I'd like the copy and wrap_copy methods better if they used the same convention as ptr::copy: first source, then destination. This is something that changed in ptr::copy half a year ago or so. Because changing that around might touch a lot of code not in this PR(?), we can wait and handle it separately.

bluss · 2015-09-22T21:48:31Z

Reminder: the PR is marked with merge conflicts

mystor · 2015-09-22T23:06:58Z

Should have all of the suggested fixes applied :)

bluss · 2015-09-22T23:39:58Z

src/libcollections/vec_deque.rs

+        // the drain.
+        //
+        //        T   t   h   H
+        // [. . . o o x x o o . . .]


bluss · 2015-09-22T23:58:13Z

reviewers can send this off with r=me as soon as the sync/send test is fixed

Gankra · 2015-09-23T04:19:58Z

@mystor I'm so sorry that it took us so long!

@bluss Thank you for being the hero this PR deserved.

bluss · 2015-09-24T20:46:27Z

Ah, with drain now using raw pointers, it needs the manual Send / Sync impls.

impls identical with Vec's Drain should be fine:

unsafe impl<'a, T: Sync> Sync for Drain<'a, T> {}
unsafe impl<'a, T: Send> Send for Drain<'a, T> {}

mystor · 2015-09-24T21:07:32Z

Yup! I already have a patch with that, but haven't had the time to push it - I'll do that hopefully tonight.

mystor · 2015-09-24T22:07:13Z

@bluss Actually, shouldn't it be

unsafe impl<'a, T: Send> Send for Drain<'a, T> {}
unsafe impl<'a, T: Send> Sync for Drain<'a, T> {}

Because Drain transfers ownership, so T must be send, but need not be sync?

mystor · 2015-09-24T22:13:20Z

@bluss Actually, can't you have an unconstrained Sync? as you can't do anything with a &Drain<'a, T> except get the length of the drained range...

bluss · 2015-09-25T00:14:26Z

I agree with the analysis but I think we should use the Sync bound because 1) It's how all other iterators do and 2) Without T: Sync it restricts which methods could ever be added (no element reading &self methods; an element reading Debug impl is actually plausible in the future).

The drain -> drain(..) fix for the sync send test is still missing by the way.

Gankra · 2015-09-25T00:16:08Z

(specifically, unconstrained sync is basically relying on &Iterator being parametric -- usually true, but hard to guard against)

mystor · 2015-10-06T13:34:22Z

Sorry for taking so long to make the 1-line change. I completely forgot about this PR :S.

@bluss or @gankro I think this can be merged then when the checks pass.

bluss · 2015-10-06T14:28:02Z

You need to update test/run-pass/sync-send-iterators-in-libcollections.rs too (See travis build log)

bluss · 2015-10-17T12:28:49Z

See the travis log, some errors, this is the first:

src/libcollectionstest/vec_deque.rs:478:30: 478:37 error: this function takes 1 parameter but 0 parameters were supplied [E0061]
src/libcollectionstest/vec_deque.rs:478             let mut iter = d.drain();
                                                                     ^~~~~~~

bluss · 2015-10-20T10:53:23Z

Yay! (There is no notification on pushes fyi, so a message on the PR calls attention to it).

bluss · 2015-10-20T10:53:28Z

@bors r+

bors · 2015-10-20T10:53:29Z

📌 Commit dec0ea0 has been approved by bluss

bors · 2015-10-20T11:55:18Z

⌛ Testing commit dec0ea0 with merge e7eb7d5...

This is a WIP PR for my implementation of drain over the VecDeque data structure supporting ranges. It brings the VecDeque drain implementation in line with Vec's. Tests haven't been written for the new function yet.

Gankra · 2015-10-20T11:57:21Z

T H E D R E A M I S R E A L

🍶

bors · 2015-10-20T13:39:21Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

bluss · 2015-10-20T13:41:05Z

🔔 🔔 drain is in

rust-highfive assigned Gankra Aug 12, 2015

Gankra mentioned this pull request Aug 13, 2015

Tracking issue for drain stabilization #27711

Closed

Gankra reviewed Aug 20, 2015
View reviewed changes

bluss assigned bluss and unassigned Gankra Aug 31, 2015

bluss reviewed Sep 22, 2015
View reviewed changes

Implement drain over a range for VecDeque

45be6fc

mystor force-pushed the vecdeque_drain_range branch from ff9793b to 45be6fc Compare September 22, 2015 22:57

bluss reviewed Sep 22, 2015
View reviewed changes

src/libcollections/vec_deque.rs

// the drain.

//

// T t h H

// [. . . o o x x o o . . .]

Copy link

Member

bluss Sep 22, 2015

Choose a reason for hiding this comment

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

🌟

mystor force-pushed the vecdeque_drain_range branch from 1c29250 to 5fa3208 Compare October 6, 2015 13:32

Add unsafe Send and Sync impls to VecDeque's Drain<T>

493355d

mystor force-pushed the vecdeque_drain_range branch from 5fa3208 to 493355d Compare October 8, 2015 13:49

Correct drain implementations in libcollectionstest

dec0ea0

bors merged commit dec0ea0 into rust-lang:master Oct 20, 2015

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement drain over a range for VecDeque #27723

Implement drain over a range for VecDeque #27723

mystor commented Aug 12, 2015

rust-highfive commented Aug 12, 2015

bluss commented Aug 14, 2015

bors commented Aug 16, 2015

Gankra Aug 20, 2015

bluss Aug 20, 2015

Gankra Aug 20, 2015

bluss Sep 22, 2015

bluss commented Aug 31, 2015

mystor commented Sep 22, 2015

bluss commented Sep 22, 2015

bluss Sep 22, 2015

bluss commented Sep 22, 2015

bluss commented Sep 22, 2015

mystor commented Sep 22, 2015

bluss Sep 22, 2015

bluss commented Sep 22, 2015

Gankra commented Sep 23, 2015

bluss commented Sep 24, 2015

mystor commented Sep 24, 2015

mystor commented Sep 24, 2015

mystor commented Sep 24, 2015

bluss commented Sep 25, 2015

Gankra commented Sep 25, 2015

mystor commented Oct 6, 2015

bluss commented Oct 6, 2015

bluss commented Oct 17, 2015

bluss commented Oct 20, 2015

bluss commented Oct 20, 2015

bors commented Oct 20, 2015

bors commented Oct 20, 2015

Gankra commented Oct 20, 2015

bors commented Oct 20, 2015

bluss commented Oct 20, 2015

Implement drain over a range for VecDeque #27723

Implement drain over a range for VecDeque #27723

Conversation

mystor commented Aug 12, 2015

rust-highfive commented Aug 12, 2015

bluss commented Aug 14, 2015

bors commented Aug 16, 2015

Gankra Aug 20, 2015

Choose a reason for hiding this comment

bluss Aug 20, 2015

Choose a reason for hiding this comment

Gankra Aug 20, 2015

Choose a reason for hiding this comment

bluss Sep 22, 2015

Choose a reason for hiding this comment

bluss commented Aug 31, 2015

mystor commented Sep 22, 2015

bluss commented Sep 22, 2015

bluss Sep 22, 2015

Choose a reason for hiding this comment

bluss commented Sep 22, 2015

bluss commented Sep 22, 2015

mystor commented Sep 22, 2015

bluss Sep 22, 2015

Choose a reason for hiding this comment

bluss commented Sep 22, 2015

Gankra commented Sep 23, 2015

bluss commented Sep 24, 2015

mystor commented Sep 24, 2015

mystor commented Sep 24, 2015

mystor commented Sep 24, 2015

bluss commented Sep 25, 2015

Gankra commented Sep 25, 2015

mystor commented Oct 6, 2015

bluss commented Oct 6, 2015

bluss commented Oct 17, 2015

bluss commented Oct 20, 2015

bluss commented Oct 20, 2015

bors commented Oct 20, 2015

bors commented Oct 20, 2015

Gankra commented Oct 20, 2015

bors commented Oct 20, 2015

bluss commented Oct 20, 2015