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.

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

Use ptr::drop_in_place for VecDeque::truncate and VecDeque::clear #65933

Merged
merged 4 commits into from Nov 11, 2019

Conversation

@crgl
Copy link
Contributor

crgl commented Oct 29, 2019

This commit allows VecDeque::truncate to take advantage of its (largely) contiguous memory layout and is consistent with the change in #64432 for Vec. As with the change to Vec::truncate, this changes both:

  • the drop order, from back-to-front to front-to-back
  • the behavior when dropping an element panics

For consistency, it also changes the behavior when dropping an element panics for VecDeque::clear.

These changes in behavior can be observed. This example (playground)

use std::collections::VecDeque;

fn main() {
    struct Bomb(usize);
    impl Drop for Bomb {
        fn drop(&mut self) {
            panic!(format!("{}", self.0));
        }
    }
    let mut v = VecDeque::from(vec![Bomb(0), Bomb(1)]);
    std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        v.truncate(0);
    }));
    std::mem::forget(v);
}

panics printing 1 today and succeeds. v.clear() panics printing 0 today and succeeds. With the change, v.clear(), v.truncate(0), and dropping the VecDeque all panic printing 0 first and then abort with a double-panic printing 1.

The motivation for this was making VecDeque::truncate more efficient since it was used in the implementation of VecDeque::clone_from (#65069), but it also makes behavior more consistent within the VecDeque and with Vec if that change is accepted (this probably doesn't make sense to merge if not).

This might need a crater run and an FCP as well.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 29, 2019

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Oct 29, 2019

Going to move this for to r? @alexcrichton as this is a libs team question.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 30, 2019

Implementation looks reasonable to me and I think the change also makes sense, thanks @crgl!

Gonna do a query of other libs folks to ensure we're all on board with the semantic changes here, which are in line with the recent semantic changes we made to Vec

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 30, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 31, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 9, 2019

Ping from triage - this has sat idle for a week
@crgl @clarfon does this need code changes or is it ready for review + merge?

cc: @alexcrichton

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Nov 9, 2019

I don't think it needs any changes.

@crgl

This comment has been minimized.

Copy link
Contributor Author

crgl commented Nov 9, 2019

I think it should be good to merge then, thanks!

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 10, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 11, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 11, 2019

📌 Commit 27e0ab5 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 11, 2019

⌛️ Testing commit 27e0ab5 with merge bc0e288...

bors added a commit that referenced this pull request Nov 11, 2019
Use ptr::drop_in_place for VecDeque::truncate and VecDeque::clear

This commit allows `VecDeque::truncate` to take advantage of its (largely) contiguous memory layout and is consistent with the change in #64432 for `Vec`. As with the change to `Vec::truncate`, this changes both:

- the drop order, from back-to-front to front-to-back
- the behavior when dropping an element panics

For consistency, it also changes the behavior when dropping an element panics for `VecDeque::clear`.

These changes in behavior can be observed. This example ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d0b1f2edc123437a2f704cbe8d93d828))
```rust
use std::collections::VecDeque;

fn main() {
    struct Bomb(usize);
    impl Drop for Bomb {
        fn drop(&mut self) {
            panic!(format!("{}", self.0));
        }
    }
    let mut v = VecDeque::from(vec![Bomb(0), Bomb(1)]);
    std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        v.truncate(0);
    }));
    std::mem::forget(v);
}
```
panics printing `1` today and succeeds. `v.clear()` panics printing `0` today and succeeds. With the change, `v.clear()`, `v.truncate(0)`, and dropping the `VecDeque` all panic printing `0` first and then abort with a double-panic printing `1`.

The motivation for this was making `VecDeque::truncate` more efficient since it was used in the implementation of `VecDeque::clone_from` (#65069), but it also makes behavior more consistent within the `VecDeque` and with `Vec` if that change is accepted (this probably doesn't make sense to merge if not).

This might need a crater run and an FCP as well.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 11, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing bc0e288 to master...

@bors bors added the merged-by-bors label Nov 11, 2019
@bors bors merged commit 27e0ab5 into rust-lang:master Nov 11, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191029.37 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.