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

Implement TrustedRandomAccess for vec::Drain #81617

Closed
wants to merge 3 commits into from
Closed

Implement TrustedRandomAccess for vec::Drain #81617

wants to merge 3 commits into from

Conversation

sdroege
Copy link
Contributor

@sdroege sdroege commented Feb 1, 2021

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2021
@sdroege sdroege mentioned this pull request Feb 1, 2021
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@sdroege sdroege force-pushed the vec-drain-trusted-random-access branch from 020dfb8 to 1d33db3 Compare February 1, 2021 08:36
@oli-obk oli-obk added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 1, 2021
@Mark-Simulacrum
Copy link
Member

r? @m-ou-se for reassignment

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2021
@Dylan-DPC-zz
Copy link

r? @Amanieu

@rust-highfive rust-highfive assigned Amanieu and unassigned m-ou-se Mar 20, 2021
@Amanieu
Copy link
Member

Amanieu commented Mar 21, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2021

📌 Commit 1d33db3 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2021
@bors
Copy link
Contributor

bors commented Mar 21, 2021

⌛ Testing commit 1d33db3 with merge e5642fd10b149d75caa992b4327123129d14f902...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 21, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 21, 2021
@JohnTitor JohnTitor added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jul 27, 2021
@JohnTitor
Copy link
Member

Marking as S-blocked as per #81617 (comment).

the8472
the8472 reviewed Jul 29, 2021
View reviewed changes
library/alloc/src/vec/drain.rs Outdated
//
// T: Copy as approximation for !Drop since get_unchecked does not advance self.iter
// and as a result the `Drop` impl above would otherwise cause items to be dropped twice.
unsafe impl<T, A: Allocator> TrustedRandomAccess for Drain<'_, T, A>
Copy link
Member

Choose a reason for hiding this comment

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

Now that #85874 has landed this should become TrustedRandomAccessNoCoerce

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched over to TrustedRandomAccessNoCoerce. Thanks and sorry for the delay!

@the8472 the8472 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 29, 2021
@crlf0710
Copy link
Member

Ping from triage, any updates on this?

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 2, 2021
@JohnCSimon
Copy link
Member

@sdroege
Ping again from triage, any updates on this?

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2021
@sdroege sdroege force-pushed the vec-drain-trusted-random-access branch from 9c70fcb to 19480c1 Compare October 10, 2021 18:21
@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2021
@GuillaumeGomez
Copy link
Member

Ready for a new round of review! :)

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2021
the8472
the8472 requested changes Nov 5, 2021
View reviewed changes
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

Taking over review of this.

Overall it seems safe, just the safety comment could use some expansion

r? @the8472

library/alloc/src/vec/drain.rs
// requires each index to be accessed only once, this is safe to do here.
//
// TrustedRandomAccess (without NoCoerce) must not be implemented because
// subtypes/supertypes of `T` might not be `NonDrop`
Copy link
Member

Choose a reason for hiding this comment

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

Drain is a bit more complicated than IntoIter due to its Drop impl doing actual work even for T: Copy. This comment should mention why this is still ok. Something along the lines that the tail move does not depend on how much has been drained.

@the8472 the8472 assigned the8472 and unassigned Amanieu Nov 5, 2021
@the8472 the8472 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@sdroege Can you please address the comment from the8472 and adjust the label with @rustbot ready when you're ready for review

@bors
Copy link
Contributor

bors commented Dec 9, 2021

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

@JohnCSimon
Copy link
Member

Ping from triage:
@sdroege
I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thank you.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jan 30, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the8472 the8472 the8472 requested changes

Assignees

@the8472 the8472

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library 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.