rust-lang / wg-allocators

Home of the Allocators working group: Paving a path for a standard set of allocator traits to be used in collections!
http://bit.ly/hello-wg-allocators
205 stars 9 forks source link

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

Closed TimDiekmann closed 3 years ago

TimDiekmann commented 3 years ago

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 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 commented 3 years ago

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.