Skip to content
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

bevy_ecs address trivial cases of unsafe_op_in_unsafe_fn #11861

Merged

Conversation

tguichaoua
Copy link
Contributor

Objective

Solution

Fix unsafe_op_in_unsafe_fn in bevy_ecs for trivial cases, i.e., add an unsafe block when the safety comment already exists or add a comment like "The invariants are uphold by the caller".

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work :) And yeah, great idea to split this out.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Feb 14, 2024
@james7132 james7132 self-requested a review February 14, 2024 17:11
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 14, 2024
@alice-i-cecile
Copy link
Member

Waiting for James to take a look before merging.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this is a step in the right direction given how dense some of the unsafe code is in bevy_ecs. However, I lament how it impacts end-to-end readabilty though given that some of these functions are nothing but unsafe, all the way through.

cargo geiger is going to explode on bevy_ecs from this PR though.

crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
tguichaoua and others added 5 commits February 19, 2024 10:43
Co-authored-by: James Liu <contact@jamessliu.com>
Co-authored-by: James Liu <contact@jamessliu.com>
Co-authored-by: James Liu <contact@jamessliu.com>
@tguichaoua
Copy link
Contributor Author

I lament how it impacts end-to-end readabilty though given that some of these functions are nothing but unsafe, all the way through.

unsafe block is a syntactic salt to force the developer to be more careful when making changes to unsafe code. Since it is a human's responsibility to ensure the safety rules, IMHO, it's worth it to pay the cost of readability in exchange for having a well-documented code.

@tguichaoua tguichaoua requested a review from james7132 February 19, 2024 15:48
@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 19, 2024
@james7132 james7132 enabled auto-merge February 21, 2024 23:52
@james7132 james7132 added this pull request to the merge queue Feb 22, 2024
Merged via the queue into bevyengine:main with commit 33c7a22 Feb 22, 2024
23 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Feb 22, 2024
…e#11861)

# Objective

- Part of bevyengine#11590
- Fix `unsafe_op_in_unsafe_fn` for trivial cases in bevy_ecs

## Solution

Fix `unsafe_op_in_unsafe_fn` in bevy_ecs for trivial cases, i.e., add an
`unsafe` block when the safety comment already exists or add a comment
like "The invariants are uphold by the caller".

---------

Co-authored-by: James Liu <contact@jamessliu.com>
@tguichaoua tguichaoua deleted the ecs/unsafe_op_in_unsafe_fn/trivial_pass branch February 22, 2024 08:07
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
…e#11861)

# Objective

- Part of bevyengine#11590
- Fix `unsafe_op_in_unsafe_fn` for trivial cases in bevy_ecs

## Solution

Fix `unsafe_op_in_unsafe_fn` in bevy_ecs for trivial cases, i.e., add an
`unsafe` block when the safety comment already exists or add a comment
like "The invariants are uphold by the caller".

---------

Co-authored-by: James Liu <contact@jamessliu.com>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
…e#11861)

# Objective

- Part of bevyengine#11590
- Fix `unsafe_op_in_unsafe_fn` for trivial cases in bevy_ecs

## Solution

Fix `unsafe_op_in_unsafe_fn` in bevy_ecs for trivial cases, i.e., add an
`unsafe` block when the safety comment already exists or add a comment
like "The invariants are uphold by the caller".

---------

Co-authored-by: James Liu <contact@jamessliu.com>
NiseVoid pushed a commit to NiseVoid/bevy that referenced this pull request Jul 8, 2024
…e#11861)

- Part of bevyengine#11590
- Fix `unsafe_op_in_unsafe_fn` for trivial cases in bevy_ecs

Fix `unsafe_op_in_unsafe_fn` in bevy_ecs for trivial cases, i.e., add an
`unsafe` block when the safety comment already exists or add a comment
like "The invariants are uphold by the caller".

---------

Co-authored-by: James Liu <contact@jamessliu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants