-
Notifications
You must be signed in to change notification settings - Fork 14k
implement VecDeque extend_from_within and prepend_from_within #147161
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
bddd849 to
af568e6
Compare
There was a problem hiding this comment.
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... 😉
| assert_eq!( | ||
| clone_counts.each_ref().map(|c| { | ||
| let old = c.get(); | ||
| c.set(0); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
| 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(), | ||
| ); |
There was a problem hiding this comment.
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
capacityis contained withinwrapped_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.
There was a problem hiding this comment.
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.
5b6f437 to
a3ebd87
Compare
|
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. |
a3ebd87 to
63bb238
Compare
|
I added extra comments in the tests and the |
|
@rustbot label -S-waiting-on-author +S-waiting-on-review |
|
Alright, let's merge it! |
…thin, r=joboet implement VecDeque extend_from_within and prepend_from_within Tracking issue: rust-lang#146975
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
…thin, r=joboet implement VecDeque extend_from_within and prepend_from_within Tracking issue: rust-lang#146975
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
…thin, r=joboet implement VecDeque extend_from_within and prepend_from_within Tracking issue: rust-lang#146975
…thin, r=joboet implement VecDeque extend_from_within and prepend_from_within Tracking issue: rust-lang#146975
…thin, r=joboet implement VecDeque extend_from_within and prepend_from_within Tracking issue: rust-lang#146975
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
Tracking issue: #146975