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

Put #[repr(transparent)] attr to bevy_ptr types #9068

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

fgrust
Copy link
Contributor

@fgrust fgrust commented Jul 8, 2023

Objective

Fix #9064

Solution


Changelog

Enhanced: bevy_ptr types would be FFI-sefe

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

#[derive(Copy, Clone)]
#[repr(transparent)]
Copy link
Member

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? 🤔

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NiklasEi thought?

Copy link
Member

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

Copy link
Contributor

@rlidwka rlidwka Jul 14, 2023

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>,
}

@Selene-Amanita Selene-Amanita added P-Unsound A bug that results in undefined compiler behavior A-Pointers Relating to Bevy pointer abstractions labels Jul 9, 2023
@fgrust fgrust requested a review from NiklasEi July 10, 2023 19:55
@NiklasEi NiklasEi 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 Jul 13, 2023
@cart cart added this pull request to the merge queue Jul 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 13, 2023
@fgrust
Copy link
Contributor Author

fgrust commented Jul 13, 2023

lint checks fail due to issue #9150

@cart cart added this pull request to the merge queue Jul 14, 2023
Merged via the queue into bevyengine:main with commit ede5848 Jul 14, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Pointers Relating to Bevy pointer abstractions P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add #[repr(transparent)] to bevy_ptr types
6 participants