Skip to content

Change parameters of AllocRef to take NonNull<[u8]>, usize instead of NonNull<u8>, Layout #69

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

Closed
TimDiekmann opened this issue Aug 11, 2020 · 1 comment
Labels
A-Alloc Trait Issues regarding the Alloc trait Proposal

Comments

@TimDiekmann
Copy link
Member

TimDiekmann commented Aug 11, 2020

Currently, AllocRef contains those signatures (leaving out _zeroed variants):

fn alloc(Layout) -> Result<NonNull<[u8]>, AllocErr>;
unsafe fn dealloc(&mut self, NonNull<u8>, Layout);
unsafe fn grow/shrink(&mut self, NonNull<u8>, Layout, usize) -> Result<NonNull<[u8]>, AllocErr>;

I like to propose to change those to take NonNull<[u8]> as they also return a slice. Also this would change the Layout parameter to take the alignment instead.

fn alloc(Layout) -> Result<NonNull<[u8]>, AllocErr>;
unsafe fn dealloc(&mut self, NonNull<[u8]>, align: usize);
unsafe fn grow/shrink(&mut self, NonNull<[u8]>, align: usize, new_size: usize) -> Result<NonNull<[u8]>, AllocErr>;

This would also change the Memory fitting safety section wording a bit:

  • The block must be allocated with the same alignment as align, and
  • The provided ptr.len() must fall in the range min ..= max, where:
    • min is the size of the layout most recently used to allocate the block, and
    • max is the latest actual size returned from alloc, grow, or shrink.

The first condition implies, that the alignment has to meet the Layout::from_size_align requirements.

The main advantage is, that one can simply pass a returned pointer back to the allocator, which will always fit the memory block. While this don't lift any safety conditions, it's more safe to use it. For structs storing only a NonNull<T> (like Box), a NonNull<[T]> can still be constructed with NonNull::slice_from_raw_parts(self.0, mem::align_of::<T>()), regardless of the returned ptr.len(), as it was requested with Layout::new<T>().

A minor downside are the parameters in grow and shrink as they take two usizes in a row. While the parameter order should be intuitive, this would be resolved, if we decide to allow reallocation to a different alignment (#5). Then, those methods would look like this:

unsafe fn grow/shrink(&mut self, NonNull<[u8]>, usize, Layout) -> Result<NonNull<[u8]>, AllocErr>;
@Amanieu
Copy link
Member

Amanieu commented Aug 12, 2020

I feel that making this change is going to make the API harder to use since you now need to construct a NonNull<[u8]>. I additionally we lose the static check that the alignment is a power of two.

I don't feel that the main advantage is valid since you're never going to pass the pointer back the allocator as it is: you're always going to cast it to some other pointer type when you allocate something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Alloc Trait Issues regarding the Alloc trait Proposal
Projects
None yet
Development

No branches or pull requests

2 participants