use ManuallyDrop instead of forget inside collections #70766
Conversation
This commit changes some usage of mem::forget into mem::ManuallyDrop in some Vec, VecDeque, BTreeMap and Box methods. Before the commit, the generated IR for some of the methods was longer, and even after optimization, some unwinding artifacts were still present.
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
About the IR, I tried with a simplified version of |
|
r? @RalfJung |
| @@ -4,9 +4,10 @@ use core::fmt::Debug; | |||
| use core::hash::{Hash, Hasher}; | |||
| use core::iter::{FromIterator, FusedIterator, Peekable}; | |||
| use core::marker::PhantomData; | |||
| use core::mem::{self, ManuallyDrop}; | |||
There was a problem hiding this comment.
Cc @Mark-Simulacrum @ssomers for BTreeMap changes
There was a problem hiding this comment.
I hardly understand ManuallyDrop, but isn't it a good fit for btree::node::Root? I don't really understand Root's comment "Note that this does not have a destructor," to begin with. Doesn't Rust generate a destructor if you don't implement Drop? So the comment should say: "Note that we (hopefully) do not invoke the generated destructor, because we need to clean up the entire tree and we do that in a fancy and dangerous way while iterating".
There was a problem hiding this comment.
I suppose that comment just means that there is no Drop impl that would clean up the data behind the raw pointers in Root -- usually one would expect such an impl to exist.
There was a problem hiding this comment.
Okay. But is that something more formally expressed by making Root.node ManuallyDrop? I should just try it and learn.
There was a problem hiding this comment.
No, putting a raw pointer (or Unique) behind manually drop is pointless.
| @@ -579,11 +578,10 @@ impl<T> RawVec<T, Global> { | |||
| "`len` must be smaller than or equal to `self.capacity()`" | |||
| ); | |||
|
|
|||
| let me = ManuallyDrop::new(self); | |||
| // NOTE: not calling `capacity()` here; actually using the real `cap` field! | |||
There was a problem hiding this comment.
Any idea what is up with that comment? cap is not used here, before not after your change.
There was a problem hiding this comment.
Looks like just an outdated comment then. Do you want to remove it?
There was a problem hiding this comment.
This PR is already r+-and-rollup-ed, so I'll remove the comment once the PR is merged to avoid merge issues.
There was a problem hiding this comment.
Ah I'll just add it to #70776 then
There was a problem hiding this comment.
Yes, the comment is outdated and I forgot to remove it.
|
I'm always in favor of replacing This looks good to me, but I am not a libs expert, so I Cc'd some. |
|
I would also be interested in an answer to #70766 (comment), but that's unrelated to this PR. @bors r=Mark-Simulacrum,RalfJung |
|
📌 Commit 2b718e8 has been approved by |
bors
added
S-waiting-on-bors
…Mark-Simulacrum,RalfJung use ManuallyDrop instead of forget inside collections This PR changes some usage of `mem::forget` into `mem::ManuallyDrop` in some `Vec`, `VecDeque`, `BTreeMap` and `Box` methods. Before the commit, the generated IR for some of the methods was longer, and even after optimization, some unwinding artifacts were still present.
Rollup of 6 pull requests Successful merges: - rust-lang#70635 (rustc_target: Some cleanup to `no_default_libraries`) - rust-lang#70748 (Do not disable field reordering on enums with big discriminant) - rust-lang#70752 (Add slice::fill) - rust-lang#70766 (use ManuallyDrop instead of forget inside collections) - rust-lang#70768 (macro_rules: `NtLifetime` cannot start with an identifier) - rust-lang#70783 (comment refers to removed type) Failed merges: r? @ghost
This PR changes some usage of
mem::forgetintomem::ManuallyDropin someVec,VecDeque,BTreeMapandBoxmethods.Before the commit, the generated IR for some of the methods was longer, and even after optimization, some unwinding artifacts were still present.