Use NonNull in the allocator API #49608
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
SimonSapin
reviewed
Apr 3, 2018
src/liballoc/boxed.rs
Outdated
| unsafe { | ||
| let p = unsafe { | ||
| if layout.size() == 0 { | ||
| NonNull::new_unchecked(mem::align_of::<T>() as *mut u8) |
There was a problem hiding this comment.
Maybe NonNull::<T>::dangling().cast()?
There was a problem hiding this comment.
Note that #48333 removes this code.
There was a problem hiding this comment.
But there's another in raw_vec.rs.
SimonSapin
reviewed
Apr 3, 2018
src/liballoc/heap.rs
Outdated
| use core::heap; | ||
| pub use core::heap::{AllocErr, CannotReallocInPlace, CollectionAllocErr, Layout}; | ||
| #[cfg(not(stage0))] | ||
| pub use core::heap::{Alloc, Excess}; | ||
| #[doc(hidden)] | ||
| pub mod __core { |
There was a problem hiding this comment.
Can #[cfg(stage0)] be added to this __core module?
There was a problem hiding this comment.
Oh right, because the only use of that __core was the #[global_allocator] expansion. Neat!
pietroalbini
added
the
S-waiting-on-author
The change looks good to me. As you said, since we have more breaking changes upcoming it might be better to batch them so that Nightly users only need to do one migration. I’m working on |
SimonSapin
added
S-blocked
|
☔ The latest upstream changes (presumably #48333) made this pull request unmergeable. Please resolve the merge conflicts. |
As discussed in rust-lang#32838 (comment) and following, instead of *mut pointers, use NonNull for the allocator API. One issue is that older rustc versions, used to bootstrap the compiler, expands #[global_allocator], used in various places including libstd or librustc_[almt]san, to code that uses the Alloc trait, so changes to that trait make bootstrapping fail. Thankfully, it does so through the location of the Alloc trait before 94d1970 so we can use at our advantage by making stage0 expose the old API as alloc::heap::Alloc. At the same time, we change the expansion for #[global_allocator] to use the new trait location under core, which will allow newer versions of rustc to bootstrap stage0 as well, despite the workaround described above.
|
Rebased on top of #48333. |
|
Also, addressed review comments. |
|
#49669 includes this change (and more). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #32838 (comment)
and following, instead of *mut pointers, use NonNull for the allocator
API.
One issue is that older rustc versions, used to bootstrap the compiler,
expands #[global_allocator], used in various places including libstd or
librustc_[almt]san, to code that uses the Alloc trait, so changes to
that trait make bootstrapping fail. Thankfully, it does so through the
location of the Alloc trait before 94d1970
so we can use at our advantage by making stage0 expose the old API as
alloc::heap::Alloc.
At the same time, we change the expansion for #[global_allocator] to use
the new trait location under core, which will allow newer versions of
rustc to bootstrap stage0 as well, despite the workaround described above.
The downside is that this is a breaking change, albeit, for an unstable feature,
that will pile up with the upcoming GlobalAlloc change. It /might/ make sense to
wait for GlobalAlloc to go in first, so that
#[global_allocator]only breaks once.But I still wanted to put this out there, get feedback on the change itself. It will be
trivial to rebase on top of GlobalAlloc if we decide to wait for that.
Cc @SimonSapin @alexcrichton