Implement VecDeque::extract_if by tisonkun · Pull Request #147780 · rust-lang/rust (original) (raw)

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.

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

Already on GitHub?Sign in to your account

Conversation31 Commits1 Checks11 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 16, 2025

r? @joboet

rustbot has assigned @joboet.
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

This comment has been minimized.

This is now ready for review. I'm trying to port some tests from LinkedList's extract_if cases.

This comment has been minimized.

// We use copy for move, and never touch this element again.
unsafe {
let hole_slot = self.vec.ptr().add(idx - self.del);
ptr::copy_nonoverlapping(cur, hole_slot, 1);

Choose a reason for hiding this comment

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

Should this instead be wrap_copy?

Choose a reason for hiding this comment

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

Yes correct. Try fix in a7d0a43.

Choose a reason for hiding this comment

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

CI looks happy. I'm not sure how to ensure the manner, i.e., constructing a VecDeque that wrap the underlying vec.

@joboet joboet left a comment • Loading

Choose a reason for hiding this comment

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

I think the algorithm here could be improved – e.g. it's not always necessary to backshift the remaining items, as one can just modify the head – but that's out of scope for this PR.

Some small nits, this looks fine otherwise.

View changes since this review

joboet 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

Oct 20, 2025

@joboet Thanks for your review! I'd integrate the comments later and ping you when it's ready for another review.

@joboet All comments address.

I found VecDeque::rotate_left that can easily make a non-contiguous deque and the private VecDeque::is_contiguous for testing assertions.

The current test case is trivial non-contiguous. If you have other edge cases in mind, please describe them so I can add them.

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

Oct 27, 2025

✌️ @tisonkun, you can now approve this pull request!

If @joboet told you to "r=me" after making some further change, please make that change, then do @bors r=@joboet

@joboet would you suggest squash all commits into one, or in logical several, e.g. impl+test?

I'd prefer one, and IIRC GitHub's squash and merge would do as this, not sure how rustbot would do.

One is fine, but as you choose. Eight is a little too many though, there's no need to preserve every little step.

Signed-off-by: tison wander4096@gmail.com

This PR was rebased onto a different master 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.

📌 Commit 06a2e72 has been approved by joboet

It is now in the queue for this repository.

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

Oct 31, 2025

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 31, 2025

bors added a commit that referenced this pull request

Oct 31, 2025

Rollup of 7 pull requests

Successful merges:

#139310 (add first HelenOS compilation targets)
#144420 (smart pointer (try_)map)
#145974 (Stabilize -Zno-jump-tables into -Cjump-tables=bool)
#147161 (implement VecDeque extend_from_within and prepend_from_within)
#147780 (Implement VecDeque::extract_if)
#148319 (docs: Fix argument names for carrying_mul_add)
#148322 (Enable file locking support in illumos)

r? @ghost @rustbot modify labels: rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 31, 2025

bors added a commit that referenced this pull request

Oct 31, 2025

Rollup of 9 pull requests

Successful merges:

#139310 (add first HelenOS compilation targets)
#147161 (implement VecDeque extend_from_within and prepend_from_within)
#147622 (unicode_data refactors)
#147780 (Implement VecDeque::extract_if)
#147942 (Enable regression labeling aliases)
#147986 (Use fstatat() in DirEntry::metadata on Apple platforms)
#148103 (cg_llvm: Pass debuginfo_compression through FFI as an enum)
#148319 (docs: Fix argument names for carrying_mul_add)
#148322 (Enable file locking support in illumos)

r? @ghost @rustbot modify labels: rollup

Zalathar added a commit to Zalathar/rust that referenced this pull request

Nov 1, 2025

Zalathar added a commit to Zalathar/rust that referenced this pull request

Nov 1, 2025

Zalathar added a commit to Zalathar/rust that referenced this pull request

Nov 1, 2025

bors added a commit that referenced this pull request

Nov 1, 2025

Rollup of 9 pull requests

Successful merges:

#139310 (add first HelenOS compilation targets)
#147161 (implement VecDeque extend_from_within and prepend_from_within)
#147622 (unicode_data refactors)
#147780 (Implement VecDeque::extract_if)
#147942 (Enable regression labeling aliases)
#147986 (Use fstatat() in DirEntry::metadata on Apple platforms)
#148103 (cg_llvm: Pass debuginfo_compression through FFI as an enum)
#148319 (docs: Fix argument names for carrying_mul_add)
#148322 (Enable file locking support in illumos)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Nov 1, 2025

Rollup merge of #147780 - tisonkun:vec-deque-extract-if, r=joboet

Implement VecDeque::extract_if

This refers to #147750.

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request

Nov 30, 2025

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request

Nov 30, 2025

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.