-
Notifications
You must be signed in to change notification settings - Fork 12
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
Replaced *mut T by Option<NonNull<T>> in box.rs #1
Conversation
@danielhenrymantilla thank you for the PR! |
Since you were nowhere to be seen I was on standby ^^' There it is |
src/boxed.rs
Outdated
}) | ||
BoxAllocation( | ||
ptr::NonNull::new(unsafe { alloc::alloc(layout) as *mut T }) | ||
.unwrap_or_else(|| std::process::abort()) // abort on oom |
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.
how is this different from just unwrap()
?
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.
Hmm, for a long time Rust strtegy on oom was to abort (since a panic!
unwinds the stack, running destructors, which may allocate memory), but now that you point it out, more "recent" threads such as rust-lang/rust#43596 seem to point in the opposite direction. I will investigate further to see if replacing it with .unwrap()
is now the preferred approach.
What’s the status on this? |
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 a good change, just needs a rebase
Normalised formatting on src/box.rs
bors r+ |
Build succeeded |
@danielhenrymantilla we just realized that this PR fixed an UB behavior on alloc() returning NULL. |
No description provided.