Change std::panicking::try::Data into a union #39721
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
It's likely the large comment under the new |
|
I thought about that but it all still seemed mostly relevant, just that the implementation changed. I was supposed to get rid of the line about it never being valid, which isn't really true any more, but I forgot, whoops. Is there anything else that seems off? |
|
@bors: r+ Thanks! |
|
📌 Commit fa5de1f has been approved by |
|
@bors: r- Er actually yeah let's touch up that comment. I can probably just use minor tweaks here and there as it's mostly still applicable |
|
Is this any better? |
src/libstd/panicking.rs
Outdated
| // * If the closure successfully returns, we write the return value into the | ||
| // data's return slot. Note that `ptr::write` is used as it's overwriting | ||
| // uninitialized data. | ||
| // * Finally, when we come back out of the `__rust_maybe_catch_panic` we're | ||
| // in one of two states: | ||
| // | ||
| // 1. The closure didn't panic, in which case the return value was | ||
| // filled in. We have to be careful to `forget` the closure, | ||
| // however, as ownership was passed to the `do_call` function. | ||
| // filled in. |
There was a problem hiding this comment.
Could still mention why not dropping is ok.
There was a problem hiding this comment.
In this case, it is dropped like normal. Well data.r is moved out, but it's just a regular move, nothing special. Should I mention that?
There was a problem hiding this comment.
Ah, I think you're right here. Your deletion seems fine.
src/libstd/panicking.rs
Outdated
| // 2. The closure panicked, in which case the return value wasn't | ||
| // filled in. In this case the entire `data` structure is invalid, | ||
| // so we forget the entire thing. | ||
| // so we ignore it because unions don't have drop glue. |
There was a problem hiding this comment.
In this case the entire `data` structure is invalid, so there is no need to drop is more correct here. We don't ignore it "because unions don't have drop glue", it is ignored by taking advantage of that.
|
Looks good to me! Want to squash the commits as well? |
No longer potentially call `mem::uninitialized::<!>()` Fixes #39432
|
Alright |
|
@bors: r+ |
|
📌 Commit ca92c51 has been approved by |
…r=alexcrichton Change std::panicking::try::Data into a union No longer potentially call `mem::uninitialized::<!>()` Fixes rust-lang#39432
ghost
deleted the
No longer potentially call
mem::uninitialized::<!>()Fixes #39432