implement rust MainThread #5671
Conversation
| //! more control over the thread is required, consider using the | ||
| //! [crate::backgroundtask::BackgroundTask] class. | ||
|
|
||
| use core::{ffi, mem, ptr}; |
There was a problem hiding this comment.
Since there's no realistic chance of ever being #![no_std], these should be imported from std instead. Also, types like NonNull, CStr, c_char, etc. should probably be imported here as well to avoid extra :: when using those types (they're used a lot in this file).
| pub(crate) unsafe fn ref_from_raw(handle: &*mut BNMainThreadAction) -> &Self { | ||
| assert!(!handle.is_null()); | ||
| mem::transmute(handle) | ||
| } |
There was a problem hiding this comment.
Is this function actually used anywhere? And since *mut T: Copy, you could reasonably just call from_raw instead and get an owned Self. Plus, elsewhere in the codebase, the return type of ref_from_raw is usually Ref<Self>. I also don't like the use of transmute here.
There was a problem hiding this comment.
This function is used by the cb_add_action. The copy is intentionally avoided here, to avoid problems like #5496
There was a problem hiding this comment.
In that case, a better alternative to raw transmute is something like:
unsafe { &*(handle as *const _ as *const Self) }It's more clear what's going on here and it doesn't tap into transmute's full power (which is a good thing)
| #[allow(clippy::mut_from_ref)] | ||
| pub(crate) unsafe fn as_raw(&self) -> &mut BNMainThreadAction { | ||
| &mut *self.handle.as_ptr() | ||
| } |
There was a problem hiding this comment.
| #[allow(clippy::mut_from_ref)] | |
| pub(crate) unsafe fn as_raw(&self) -> &mut BNMainThreadAction { | |
| &mut *self.handle.as_ptr() | |
| } | |
| pub(crate) fn as_raw(&self) -> *mut BNMainThreadAction { | |
| self.handle.as_ptr() | |
| } |
This function isn't pub and passing the raw *mut BNMainThreadAction across FFI already requires unsafe, so this function should just be a wrapper around NonNull::as_ptr. As far as I can tell, every use of this function coerces the return type from &mut to *mut.
There was a problem hiding this comment.
Similar to above, the *mut T: Copy is avoided and lifetime is enforced to avoid situations like #5496.
Although the reference is usually immediately converted into a pointer, voiding the effort of enforcing the lifetime. In any case, this is a internal function and it could be fixed without changing the API.
|
|
||
| #[repr(transparent)] | ||
| pub struct MainThreadAction { | ||
| handle: ptr::NonNull<BNMainThreadAction>, |
There was a problem hiding this comment.
Reading the documentation for NonNull mentions that it's covariant as opposed to *mut T which is invariant. This worries me a little bit especially because much of the Binja Rust bindings offer some amount of interior mutability by having methods take &self while calling C functions that mutate the underlying data. I'm unsure of the soundness here, but I think it's probably fine because the only data held on the Rust side is the raw handle. Although, since we're talking about a concrete NonNull and not a generic NonNull<T>, then variance probably doesn't even matter.
No description provided.