-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Merged by Bors] - Constify SpritePipelineKey implementation. #6976
Conversation
crates/bevy_sprite/src/render/mod.rs
Outdated
Self::from_bits(msaa_bits).unwrap() | ||
// SAFETY: The input is the correct type and the mask will ensure the output range is | ||
// capped | ||
unsafe { Self::from_bits_unchecked(msaa_bits) } |
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.
For other reviewers here's the safety comment from the bitflags crate
https://docs.rs/bitflags/latest/src/bitflags/lib.rs.html#575-577
/// The caller of `from_bits_unchecked()` has to ensure that
/// all bits correspond to a defined flag or that extra bits
/// are valid for this bitflags type.
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.
Could we add some docs for these functions? Not going to block on that though.
crates/bevy_sprite/src/render/mod.rs
Outdated
Self::from_bits(msaa_bits).unwrap() | ||
// SAFETY: The input is the correct type and the mask will ensure the output range is | ||
// capped | ||
unsafe { Self::from_bits_unchecked(msaa_bits) } |
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.
Idk if this is a good enough reason to introduce unsafe code. Does const-ing this unlock specific scenarios we need? Are there performance benefits (that we have measured)?
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.
unsafe { Self::from_bits_unchecked(msaa_bits) } | |
Self::from_bits_truncate(msaa_bits) |
This should be const
and safe.
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.
Thanks, I overlooked that, will fix it shortly.
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.
Fixed.
…s_unchecked in order to be const.
bors r+ |
# Objective - Describe the objective or issue this PR addresses. SpritePipelineKey could use more constification. ## Solution Constify SpritePipelineKey implementation. ## Changelog Co-authored-by: AxiomaticSemantics <117950168+AxiomaticSemantics@users.noreply.github.com>
Build failed (retrying...): |
# Objective - Describe the objective or issue this PR addresses. SpritePipelineKey could use more constification. ## Solution Constify SpritePipelineKey implementation. ## Changelog Co-authored-by: AxiomaticSemantics <117950168+AxiomaticSemantics@users.noreply.github.com>
# Objective - Describe the objective or issue this PR addresses. SpritePipelineKey could use more constification. ## Solution Constify SpritePipelineKey implementation. ## Changelog Co-authored-by: AxiomaticSemantics <117950168+AxiomaticSemantics@users.noreply.github.com>
# Objective - Describe the objective or issue this PR addresses. SpritePipelineKey could use more constification. ## Solution Constify SpritePipelineKey implementation. ## Changelog Co-authored-by: AxiomaticSemantics <117950168+AxiomaticSemantics@users.noreply.github.com>
Objective
SpritePipelineKey could use more constification.
Solution
Constify SpritePipelineKey implementation.
Changelog