-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Remove bevy_ptr::dangling_with_align() and inline it
#21822
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
base: main
Are you sure you want to change the base?
Conversation
|
@SkiFire13, want to take a look at this? This was your suggestion I believe. |
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
|
|
|
why do we care that the alignment is a power of 2? i.e. why don't we just use NonNull::without_provenance directly? I only see it being used in bevy here.
|
| repository = "https://github.com/bevyengine/bevy" | ||
| documentation = "https://docs.rs/bevy" | ||
| rust-version = "1.88.0" | ||
| rust-version = "1.89.0" |
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.
I'm not sure if it makes sense to bump the MSRV just for this small change.
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.
I personally don't see the harm: without_provenance() is much simpler than what we had before, and it gives Miri extra insight into what we're doing with the pointer. Bevy's MSRV policy has made it clear that we'll take advantage of newly-stabilized features if they're useful to us, so I feel it's fair game.
I don't know enough myself to speak on this matter, but it appears to be a requirement by Rust. Looking at
Given how |
… site `dangling_with_align()` is only being used once, in `bevy_ecs`, and it's only 2 lines of code! Let's squash it :)
NonNull::without_provenance() in dangling_with_align()bevy_ptr::dangling_with_align() and inline it
|
The lint fixes might belong in a different pull request, since it is a little distracting from the purpose listed in the PR title and description. Plus, if you need to revert for whatever reason if the version upgrade or the |
# Objective - In #21822 the MSRV bump triggers some new lint warnings, so this pr fixes them. ## Solution - Bumped the msrv in bevy_ecs and fixed the lints. But this pr does not actually bump the msrv. These fixes are all for combining nested if's together.
Objective
bevy_ptr::dangling_with_align()is only used once, inbevy_ecs'sBlobArray::with_capacity(), and it isn't generally useful outside of the engine's internals. We can remove the function and inline its implementation into its call site.bevy_ptr::dangling_with_align()has a TODO comment that was leftover from Remove int2ptr cast inbevy_ptr::dangling_with_alignand remove-Zmiri-permissive-provenancein CI #15311 (comment), where it was suggested thatdangling_with_align()should usewithout_provenance().with_addr(), mentioned in the TODO comment, could also be used, but it's a more roundabout solution. The reason it was mentioned was because the original author thought it would be stabilized beforewithout_provenance().Solution
dangling_with_align().NonNull::without_provenance()(since it is now stable and doesn't requireunsafeor pointer math)Testing
MIRIFLAGS="-Zmiri-strict-provenance" cargo +nightly miri testBut what is this provenance thingy?
The official docs provide a more in-depth explanation, but basically a pointer is more than just a number referring to a memory address. Pointers have permissions associated with them that track:
These permissions are pointer provenance. They aren't stored at runtime, the Rust compiler doesn't know them at compile time! Miri is the only tool that I know of that tracks provenance, which it uses to ensure pointers adheres to their spatial, temporal, and mutability permissions. That's why I mentioned Miri in the Testing section. :)