-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
added core::mem::reshape #130849
base: master
Are you sure you want to change the base?
added core::mem::reshape #130849
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @workingjubilee (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
let result = ptr::read(dest); | ||
ptr::write(dest, f(result)); |
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.
this is not sound if f
unwinds, as the value will be dropped twice. see https://docs.rs/replace_with/latest/replace_with/ for details. the design space here is nontrivial, as there are many possible behaviors to do on unwind. this should at least go through an ACP, but probably better for more general discussion on internals.rust-lang.org. i'd also suggesting searching there for previous discussion about "replace_with", which is what this function is generally called
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.
yeah I just detected that myself and ran into a wall. Sounds good 👍
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.
has there been any discussion about a feature to ensure a closure does not panic?
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.
hm. this has probably come up in vague ways in many places but nothing concrete.
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.
either way it's definitely both that will arrive any time soon, so it probably makes sense to close the PR and tracking issue for now
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.
thank you for it anyways :3
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.
👍
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.
Could you execute the closure inside an extern "C" wrapper func that would abort on unwind? I think I recently saw a PR on a helper that exposed abort on unwind (mirroring the catch_unwind function) using that technique
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.
You're thinking of #130339
/// | ||
/// let mut v: Vec<i32> = vec![1, 2]; | ||
/// | ||
/// mem::replace(&mut v, |mut v| { v.push(3); v }); |
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.
/// mem::replace(&mut v, |mut v| { v.push(3); v }); | |
/// mem::reshape(&mut v, |mut v| { v.push(3); v }); |
Should this be reshape
?
/// ``` | ||
#[inline] | ||
#[unstable(feature = "mem_reshape", issue = "130848")] | ||
pub fn reshape<T>(dest: &mut T, f: impl FnOnce(T) -> T) { |
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 don't think this is a good name. When I first came across the tracking issue in the issue list, I though this was something transmute related.
tracking issue: #130848