-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Put #[repr(transparent)]
attr to bevy_ptr types
#9068
Put #[repr(transparent)]
attr to bevy_ptr types
#9068
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
crates/bevy_ptr/src/lib.rs
Outdated
#[derive(Copy, Clone)] | ||
#[repr(transparent)] |
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 am not familiar with repr(transparent)
, but according to https://doc.rust-lang.org/nomicon/other-reprs.html#reprtransparent it should only be used on structs with a single non-zero sized field. So if I understand correctly, this should not be added to Aligned
and Unaligned
.
I guess the ptr types below are fine because phantom data has no size? 🤔
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.
alright, As far as I took reading docs, it's unnecessary to put repr(transparent)
into Aligned
and Unaligned
, which not used for FFI-boundary. Thanks for catching me up.
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.
@NiklasEi thought?
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.
Yes, I think that makes sense
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.
So if I understand correctly, this should not be added to Aligned and Unaligned.
Yes, repr(transparent)
makes no sense with ZST.
So this is wrong (not technically wrong, just useless):
#[repr(transparent)]
struct A;
I guess the ptr types below are fine because phantom data has no size?
It is okay to have any number of ZSTs in addition to real type, and this kind of code is all over physx-rs (for example, here).
So this is fine:
#[repr(transparent)]
struct B<T, U> {
data: T,
_phantom: PhantomData<U>,
}
lint checks fail due to issue #9150 |
Objective
Fix #9064
Solution
Changelog
Enhanced: bevy_ptr types would be FFI-sefe