Make vec::Drain and binary_heap::Drain covariant by tomgarcia · Pull Request #34951 · rust-lang/rust

tomgarcia · 2016-07-21T08:14:03Z

I removed all mutable pointers/references, and added covariance tests similar to the ones in #32635. It builds and passes the tests, but I noticed that there weren't any tests of Drain's behaviour (at least not in libcollectionstest), so I'm not sure if my changes accidently broke Drain's behaviour. Should I add some tests for that (and if so, what should the tests include)?

rust-highfive · 2016-07-21T08:14:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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. Due to 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.

apasel422 · 2016-07-21T11:27:02Z

Thanks for the PR! Alas, there are a few issues that need to be resolved before we can merge it:

Removing the lifetime parameter from these types is a breaking change.
This changes the runtime of Vec::drain from O(1) to O(n) and causes it to allocate.

If the libs team decides that making these types covariant is worthwhile, the change will likely just involve using alternate types for Drain's fields (such as Shared).

tomgarcia · 2016-07-22T03:56:49Z

Okay, fixed it. It's now basically the same as the original code, but with different types.

brson · 2016-07-25T21:38:13Z

@bors r+

bors · 2016-07-25T21:38:14Z

📌 Commit d1e2a93 has been approved by brson

bors · 2016-07-25T21:59:03Z

⌛ Testing commit d1e2a93 with merge 24143b9...

bors · 2016-07-25T22:03:13Z

💔 Test failed - auto-win-gnu-32-opt

alexcrichton · 2016-07-25T22:13:47Z

@bors: retry

On Mon, Jul 25, 2016 at 3:03 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5071
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#34951 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95E2psH7iVu2J9QiKVqVO2lKwy1OTks5qZTKkgaJpZM4JRjGz
.

tomgarcia · 2016-07-26T06:39:35Z

@alexcrichton How do I interpret the error log from the build? The log seems to imply the error was in building llvm, but my changes did not touch llvm.

TimNN · 2016-07-26T06:53:33Z

@tomgarcia There's nothing wrong with this PR, @bors is just having some problems at the moment and will retry this PR at some point (thats what the @bors: retry is for).

tomgarcia · 2016-07-26T06:54:59Z

Okay, thanks.

bors · 2016-07-26T10:24:25Z

⌛ Testing commit d1e2a93 with merge cb23f63...

bors · 2016-07-26T10:29:16Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

alexcrichton · 2016-07-26T17:21:09Z

@bors: retry

On Tue, Jul 26, 2016 at 3:29 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/1951
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34951 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95C7ERoIkhUK1kbPE_oa3cXDtHfnCks5qZeF_gaJpZM4JRjGz
.

bors · 2016-07-28T12:24:31Z

⌛ Testing commit d1e2a93 with merge cec262e...

Make vec::Drain and binary_heap::Drain covariant I removed all mutable pointers/references, and added covariance tests similar to the ones in #32635. It builds and passes the tests, but I noticed that there weren't any tests of Drain's behaviour (at least not in libcollectionstest), so I'm not sure if my changes accidently broke Drain's behaviour. Should I add some tests for that (and if so, what should the tests include)?

Make vec::Drain and binary_heap::Drain covariant I removed all mutable pointers/references, and added covariance tests similar to the ones in rust-lang#32635. It builds and passes the tests, but I noticed that there weren't any tests of Drain's behaviour (at least not in libcollectionstest), so I'm not sure if my changes accidently broke Drain's behaviour. Should I add some tests for that (and if so, what should the tests include)?

bors · 2016-07-28T15:19:31Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild
Approved by: brson
Pushing cec262e to master...

Rollup of 7 pull requests - Successful merges: #34951, #34963, #34969, #35013, #35037, #35040, #35058 - Failed merges:

apasel422 · 2016-07-28T19:04:42Z

@tomgarcia Any interest in making a similar change to vec_deque::Drain?

tomgarcia · 2016-07-28T19:12:01Z

Yes, I was planning to fix the other Drains. I just started with vec::Drain to make sure I knew what I was doing.

…cottmcm Remove incorrect comment in `Vec::drain` r? `@scottmcm` Turns out this comment wasn't correct for 6 years, since rust-lang#34951, which switched from using `slice::IterMut` into using `slice::Iter`.

…cottmcm Remove incorrect comment in `Vec::drain` r? ``@scottmcm`` Turns out this comment wasn't correct for 6 years, since rust-lang#34951, which switched from using `slice::IterMut` into using `slice::Iter`.

Make vec::Drain and binary_heap::Drain covariant
97d082c

rust-highfive assigned brson Jul 21, 2016

apasel422 added the T-libs-api [DEPRECATED; DO NOT USE] label Jul 21, 2016

brson added the I-nominated label Jul 21, 2016

Readding lifetime parameters and removing allocation
d1e2a93

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 25, 2016

brson removed the I-nominated label Jul 27, 2016

Manishearth mentioned this pull request Jul 28, 2016
Rollup of 7 pull requests #35086
Merged

bors added a commit that referenced this pull request Jul 28, 2016
Auto merge of#35086- Manishearth:rollup, r=Manishearth
748ecb1
Rollup of 7 pull requests - Successful merges: #34951, #34963, #34969, #35013, #35037, #35040, #35058 - Failed merges:

bors merged commit d1e2a93 into rust-lang:masterJul 28, 2016

WaffleLapkin mentioned this pull request Oct 21, 2022
Remove incorrect comment in Vec::drain#103359
Merged

steffahn mentioned this pull request Dec 14, 2024
Tracking Issue for vec::Drain{,Filter}::keep_rest#101122
Open
3 tasks

Uh oh!

Uh oh!

Conversation

tomgarcia commented Jul 21, 2016

Uh oh!

rust-highfive commented Jul 21, 2016

Uh oh!

apasel422 commented Jul 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomgarcia commented Jul 22, 2016

Uh oh!

brson commented Jul 25, 2016

Uh oh!

bors commented Jul 25, 2016

Uh oh!

bors commented Jul 25, 2016

Uh oh!

bors commented Jul 25, 2016

Uh oh!

alexcrichton commented Jul 25, 2016

Uh oh!

tomgarcia commented Jul 26, 2016

Uh oh!

TimNN commented Jul 26, 2016

Uh oh!

tomgarcia commented Jul 26, 2016

Uh oh!

bors commented Jul 26, 2016

Uh oh!

bors commented Jul 26, 2016

Uh oh!

alexcrichton commented Jul 26, 2016

Uh oh!

bors commented Jul 28, 2016

Uh oh!

bors commented Jul 28, 2016

Uh oh!

apasel422 commented Jul 28, 2016

Uh oh!

tomgarcia commented Jul 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

apasel422 commented Jul 21, 2016•
edited
Loading