Conversation
|
Requires #259 to be merged first |
ba694e4 to
b845b1f
Compare
|
@nex3 can you confirm this works for DS3 too as far as you can? |
b845b1f to
ccf97cf
Compare
dd3ce2b to
cebdf96
Compare
ccf97cf to
92d0d6c
Compare
cebdf96 to
44a19b1
Compare
92d0d6c to
183066a
Compare
2d2f960 to
dfd9996
Compare
139cb21 to
19d7f8c
Compare
a7688c1 to
f643771
Compare
| pub struct DLAllocatorForStl(NonNull<DLAllocatorBase>); | ||
|
|
||
| impl<T> DoublyLinkedList<T> { | ||
| pub fn iter(&self) -> impl Iterator<Item = &T> { | ||
| let mut count = self.count; | ||
| let mut current = unsafe { self.head.as_ref() }; | ||
| impl From<NonNull<DLAllocatorBase>> for DLAllocatorForStl { | ||
| fn from(ptr: NonNull<DLAllocatorBase>) -> Self { | ||
| Self(ptr) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is (and has always been) unsound. The From implementation especially allows you to convert from a pointer which provides no guarantees that the allocator is even initialized, let alone that it will live as long as as the reference. We should take this opportunity to fix it.
The most straightforward solution would be to just use &'static DLAllocatorBase instead for the vast majority of uses that do in fact refer to global allocators that last the entire process lifetime. That means we don't have to invent a bunch of our own custom infrastructure around creating and using it; we can just implement fromsoftware_shared_stl::Allocator for DLAllocatorBase directly.
If/when we need to model weirder situations like arena allocators that only exist for the lifetime of some parent object, we could use a newtype specifically for that like ShortLivedDLAllocator. This would also implement fromsoftware_shared_stl::Allocator to DLAllocatorBase, but it would have to strictly require (in the "Safety" block) that any ShortLivedDLAllocator members on FFI structs be private; otherwise, a caller with a mutable reference could potentially take ownership of it and use it beyond its stated lifetime without an unsafe {} block. This would have to include the STL types.
There was a problem hiding this comment.
The problem here is: allocator type in collections is actually pointer to an allocator, so implementing it for DLAllocatorBase will not work, and we can't implement anything for NonNull or use OwnedPtr to get Deref.
| } | ||
| }) | ||
| impl fromsoftware_shared_stl::Allocator for DLAllocatorForStl { | ||
| unsafe fn allocate_raw(&mut self, size: usize, allign: usize) -> *mut c_void { |
There was a problem hiding this comment.
Thinking about this more, I think we have to make this &self, not &mut self. Most allocators in these games are thread-safe, and for those it's expected that de/allocation calls be made concurrently across multiple threads. If this is &mut self that means that self can change while we hold a mutable reference, which violates soundness.
For arena allocators, we can treat them as having implicit UnsafeCells.
| impl DLAllocatorForStl { | ||
| /// Returns the global instance of DLAllocator that uses the | ||
| /// standard MSVC malloc()/free() implementation for heap management | ||
| pub fn runtime_heap_allocator() -> Self { |
There was a problem hiding this comment.
This should also return &'static DLAllocatorBase.
|
I don't think we should tie the allocator infrastructure so tightly to STL in particular. The same DLKR allocators are used all over the codebase, not just in STL types. Particularly with #269, we'll want to call the allocation and deallocation functions in contexts that are divorced from STL. In practice, I think this means that we shouldn't rename |
No description provided.