Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make vec::Drain and binary_heap::Drain covariant #34951

Merged
merged 2 commits into from
Jul 28, 2016
Merged

Make vec::Drain and binary_heap::Drain covariant #34951

merged 2 commits into from
Jul 28, 2016

Conversation

tomgarcia
Copy link
Contributor

#30642

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
Copy link
Collaborator

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
Copy link
Contributor

apasel422 commented Jul 21, 2016 •
edited
Loading

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).

@apasel422 apasel422 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 21, 2016
@tomgarcia
Copy link
Contributor Author

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

@brson
Copy link
Contributor

brson commented Jul 25, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2016

📌 Commit d1e2a93 has been approved by brson

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

bors commented Jul 25, 2016

⌛ Testing commit d1e2a93 with merge 24143b9...

@bors
Copy link
Contributor

bors commented Jul 25, 2016

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

@alexcrichton
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Contributor

TimNN commented Jul 26, 2016

@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
Copy link
Contributor Author

Okay, thanks.

@bors
Copy link
Contributor

bors commented Jul 26, 2016

⌛ Testing commit d1e2a93 with merge cb23f63...

@bors
Copy link
Contributor

bors commented Jul 26, 2016

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

@alexcrichton
Copy link
Member

@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
.

@brson brson removed the I-nominated label Jul 27, 2016
@Manishearth Manishearth mentioned this pull request Jul 28, 2016
Merged
@bors
Copy link
Contributor

bors commented Jul 28, 2016

⌛ Testing commit d1e2a93 with merge cec262e...

bors added a commit that referenced this pull request Jul 28, 2016
@bors
Auto merge of #34951 - tomgarcia:covariant-vec, r=brson
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)?
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 28, 2016
@Manishearth
Rollup merge of rust-lang#34951 - tomgarcia:covariant-vec, r=brson
80c998d 
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
Copy link
Contributor

bors commented Jul 28, 2016

☀️ 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...

bors added a commit that referenced this pull request Jul 28, 2016
@bors
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 bors merged commit d1e2a93 into rust-lang:master Jul 28, 2016
@apasel422
Copy link
Contributor

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

@tomgarcia
Copy link
Contributor Author

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

@WaffleLapkin WaffleLapkin mentioned this pull request Oct 21, 2022
Merged
notriddle added a commit to notriddle/rust that referenced this pull request Oct 21, 2022
@notriddle
Rollup merge of rust-lang#103359 - WaffleLapkin:drain_no_mut_qqq, r=s…
3ac87dd 
…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`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 21, 2022
@matthiaskrgr
Rollup merge of rust-lang#103359 - WaffleLapkin:drain_no_mut_qqq, r=s…
1b2f594 
…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`.
@steffahn steffahn mentioned this pull request Dec 14, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@brson brson

Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tomgarcia @rust-highfive @apasel422 @brson @bors @alexcrichton @TimNN