-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Clean up pointer use in BundleSpawner/BundleInserter #12269
Conversation
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
550eb5b
to
2162a77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely seems like an improvement
I wonder if it would make more sense for bevy_ptr
's non-null API to look more like a typed version of bevy_ptr::Ptr
pub struct NonNull<'a, T> {
ptr: NonNull<T>,
_ref: PhantomData<&'a T>,
}
pub struct NonNullMut<'a, T> {
ptr: NonNull<T>,
_ref: PhantomData<&'a mut T>,
}
This should allow us to enforce at compile time that the data is initialized and aligned and valid for reads/writes, so to dereference it you'd just need to guarantee there's no aliasing
Isn't that just |
I see it as an &'a mut T with the aliasing requirement lifted -- but I guess that's just |
CI is failing still <3 |
# Objective Following bevyengine#10756, we're now using raw pointers in BundleInserter and BundleSpawner. This is primarily to get around the need to split the borrow on the World, but it leaves a lot to be desired in terms of safety guarantees. There's no type level guarantee the code can't dereference a null pointer, and it's restoring them to borrows fairly liberally within the associated functions. ## Solution * Replace the pointers with `NonNull` and a new `bevy_ptr::ConstNonNull` that only allows conversion back to read-only borrows * Remove the closure to avoid potentially aliasing through the closure by restructuring the match expression. * Move all conversions back into borrows as far up as possible to ensure that the borrow checker is at least locally followed.
Objective
Following #10756, we're now using raw pointers in BundleInserter and BundleSpawner. This is primarily to get around the need to split the borrow on the World, but it leaves a lot to be desired in terms of safety guarantees. There's no type level guarantee the code can't dereference a null pointer, and it's restoring them to borrows fairly liberally within the associated functions.
Solution
NonNull
and a newbevy_ptr::ConstNonNull
that only allows conversion back to read-only borrows