[Shared] Add the ability to allocate and drop OwnedPtrs#269
[Shared] Add the ability to allocate and drop OwnedPtrs#269nex3 wants to merge 4 commits intovswarte:mainfrom
OwnedPtrs#269Conversation
| } | ||
| } | ||
|
|
||
| impl<T, A: GameAllocator> Drop for OwnedPtr<T, A> { |
There was a problem hiding this comment.
No problem with implementing Drop on OwnedPtr, but it might be worth adding a note somewhere (probably on new and the OwnedPtr type itself) that due to drop order differences between C++ and Rust you have to be careful when dropping a C++ type that is modeled as a Rust struct.
Basically if there's borrowing of data between fields of the type (or any other drop order dependencies), you have to wrap all fields in ManuallyDrop and have the Drop impl either
- Call the C++ destructor (easy if the class is virtual, annoying otherwise);
- If the C++ type does not define a custom destructor, call
ManuallyDrop::dropon the fields in reverse order.
This is especially relevant for generic structs where you can't control if a particular instantiation is drop-order sensitive.
| /// directly invoking [`panic!`] or similar. | ||
| /// | ||
| /// [`handle_alloc_error`]: std::alloc::handle_alloc_error | ||
| fn allocate(layout: Layout) -> Result<NonNull<[u8]>, AllocError>; |
There was a problem hiding this comment.
Do we plan to use this trait for more than OwnedPtr? If so, allowing a &self reference to be passed would make it compatible with some of the later game's stateful allocators. This would require making the trait unsafe and add a relevant safety section.
The big issue with this suggestion would be that OwnedPtr is no longer #[repr(transparent)] with a pointer, and have to think about how std::unique_ptr stores its deleter. In MSVC15 it's before the pointer and properly handles ZSTs using empty base optimization, but that's probably not the case in MSVC12. We don't want to add types whose layout isn't compatible with all games in the shared crate.
Maybe add &self references, rename OwnedPtr::new to new_in that takes an A and add something like
const {
if size_of::<A>() != 0 {
panic!("OwnedPtr::new_in with a stateful (non-ZST) allocator is currently forbidden; this limitation may be lifted in the future");
}
}to get compile-time panics if A isn't a ZST for now?
Co-authored-by: William Tremblay <tremwil@gmail.com>
No description provided.