Skip to content

add VecDeque::splice #147247

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
Open
Qelxiros wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add VecDeque::splice #147247

Qelxiros wants to merge 1 commit into from
+299 −13

Conversation

@Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Oct 1, 2025 •
edited by rustbot
Loading

Tracking issue: #146975

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 1, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@antonilol antonilol mentioned this pull request Oct 12, 2025
Open
7 tasks
@antonilol
Copy link
Contributor

antonilol commented Oct 12, 2025

Thanks for working on this!

@tgross35
Copy link
Contributor

tgross35 commented Oct 16, 2025

Sorry I haven't had a chance to look at this, still behind on my reviews

r? libs

@rustbot rustbot assigned Mark-Simulacrum and unassigned tgross35 Oct 16, 2025
@bors
Copy link
Collaborator

bors commented Nov 1, 2025

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

Mark-Simulacrum
Mark-Simulacrum requested changes Nov 8, 2025
View reviewed changes
library/alloc/src/collections/vec_deque/splice.rs
// This will set drain.remaining to 0, so its drop won't try to read deallocated memory on
// drop.
self.drain.by_ref().for_each(drop);

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Nov 8, 2025

Choose a reason for hiding this comment

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

Do we need something like

rust/library/alloc/src/vec/splice.rs

Line 62 in 87f9dcd

self.drain.iter = (&[]).iter();
here to avoid problems with Drain::drop?

Copy link
Contributor Author

@Qelxiros Qelxiros Nov 17, 2025

Choose a reason for hiding this comment

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

As I understand it, the for_each will ensure that remaining is set to 0, so Drain::drop won't drop anything. I intentionally use Drain::drop to put the elements together again properly.

library/alloc/src/collections/vec_deque/splice.rs
unsafe fn move_tail(&mut self, additional: usize) {
let deque = unsafe { self.deque.as_mut() };
let tail_start = deque.len + self.drain_len;
deque.buf.reserve(tail_start + self.tail_len, additional);
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Nov 8, 2025

Choose a reason for hiding this comment

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

Do we have coverage for this reserve running? Normally growing VecDeque requires extra logic AFAICT, not just reserving the underlying buffer.

Copy link
Contributor Author

@Qelxiros Qelxiros Nov 17, 2025

Choose a reason for hiding this comment

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

It should run anytime the new iterator (replace_with) has a different length than the removed elements, which is definitely tested. Do you have a more specific case in which this could cause issues?

@rustbot rustbot 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 8, 2025
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot 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 Nov 17, 2025
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Nov 17, 2025

I'm not sure why the CI isn't running, but all the requested changes should be good
@rustbot ready

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Dec 28, 2025

r=me with a rebase

@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Dec 29, 2025

@Mark-Simulacrum should be good, but I don't have permission to r=you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-Simulacrum Mark-Simulacrum Awaiting requested review from Mark-Simulacrum

1 more reviewer

@antonilol antonilol antonilol left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

6 participants

@Qelxiros @rustbot @antonilol @tgross35 @bors @Mark-Simulacrum