Skip to content

implement VecDeque extend_from_within and prepend_from_within - #147161

Merged
bors merged 1 commit into
rust-lang:masterfrom
antonilol:vec-deque-extend-from-within
Nov 1, 2025
Merged

implement VecDeque extend_from_within and prepend_from_within#147161
bors merged 1 commit into
rust-lang:masterfrom
antonilol:vec-deque-extend-from-within

Conversation

@antonilol

@antonilolantonilol commented Sep 29, 2025

Copy link
Copy Markdown
Contributor

Tracking issue: #146975

@rustbotrustbot added 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. labels Sep 29, 2025
@rust-log-analyzer

This comment has been minimized.

@antonilol
antonilol marked this pull request as ready for review September 30, 2025 19:51
@rustbotrustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2025
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 30, 2025
@rustbot

Copy link
Copy Markdown
Collaborator

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

@antonilol
antonilolforce-pushed the vec-deque-extend-from-within branch from bddd849 to af568e6CompareSeptember 30, 2025 20:39

@joboetjoboet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The implementation looks good to me, though I wouldn't mind a few more comments... 😉

View changes since this review

assert_eq!(
clone_counts.each_ref().map(|c| {
let old = c.get();
c.set(0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason why the counts are reset here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oh that's a leftover from before splitting the tests in one that needs panic = "unwind" and the rest, will remove it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find these tests incredibly hard to read. Could you test this another way? Perhaps by giving each element an ID and then testing which elements were cloned/destroyed by each operation...

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I get that, will try this.

Comment on lines +269 to +273
debug_assert!(
!(src_wraps && dst_wraps),
"BUG: at most one of src and dst can wrap. src={src} dst={dst} count={count} cap={}",
self.capacity(),
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's true that this can be derived from the function's safety invariants, but that derivation isn't really obvious. Would you mind adding a comment that explains why this must be true? Perhaps something like

Wrapping occurs if capacity is contained within wrapped_src..wrapped_src + count/wrapped_dst..wrapped_dst + count. Since these two ranges must not overlap as per the safety invariants of this function, only one range can wrap.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I like this, it is really intuitive, will add this.

@joboetjoboet 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 21, 2025
@antonilol
antonilolforce-pushed the vec-deque-extend-from-within branch from 5b6f437 to a3ebd87CompareOctober 23, 2025 11:49
@rustbot

Copy link
Copy Markdown
Collaborator

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.

@antonilol
antonilolforce-pushed the vec-deque-extend-from-within branch from a3ebd87 to 63bb238CompareOctober 23, 2025 11:53
@antonilol

Copy link
Copy Markdown
ContributorAuthor

I added extra comments in the tests and the SpecExtendFromWithin implementation for T: Clone, also rewrote the tests so they are easier to read and understand

@antonilol

Copy link
Copy Markdown
ContributorAuthor

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbotrustbot 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 23, 2025
@joboet

Copy link
Copy Markdown
Member

Alright, let's merge it!
@bors r+

@bors

bors commented Oct 31, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit 63bb238 has been approved by joboet

It is now in the queue for this repository.

@borsbors 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
…thin, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: rust-lang#146975
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
…thin, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: rust-lang#146975
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
…thin, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: rust-lang#146975
@ZalatharZalathar mentioned this pull request Nov 1, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 1, 2025
…thin, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: rust-lang#146975
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 1, 2025
…thin, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: rust-lang#146975
@ZalatharZalathar mentioned 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
@bors
bors merged commit 53c52a2 into rust-lang:masterNov 1, 2025
11 checks passed
@rustbotrustbot added this to the 1.93.0 milestone Nov 1, 2025
rust-timer added a commit that referenced this pull request Nov 1, 2025
Rollup merge of #147161 - antonilol:vec-deque-extend-from-within, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: #146975
@antonilol
antonilol deleted the vec-deque-extend-from-within branch November 1, 2025 17:20
github-actionsBot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
…thin, r=joboet
implement VecDeque extend_from_within and prepend_from_within
Tracking issue: rust-lang#146975
github-actionsBot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#139310 (add first HelenOS compilation targets)
- rust-lang#147161 (implement VecDeque extend_from_within and prepend_from_within)
- rust-lang#147622 (`unicode_data` refactors)
- rust-lang#147780 (Implement VecDeque::extract_if)
- rust-lang#147942 (Enable regression labeling aliases)
- rust-lang#147986 (Use fstatat() in DirEntry::metadata on Apple platforms)
- rust-lang#148103 (cg_llvm: Pass `debuginfo_compression` through FFI as an enum)
- rust-lang#148319 (docs: Fix argument names for `carrying_mul_add`)
- rust-lang#148322 (Enable file locking support in illumos)
r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@antonilol@rust-log-analyzer@rustbot@joboet@bors