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

Allow repr(transparent) to be used generically in derive(Pod) #139

Merged
merged 3 commits into from Nov 3, 2022
Merged

Allow repr(transparent) to be used generically in derive(Pod) #139

merged 3 commits into from Nov 3, 2022

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Oct 29, 2022

My current use case is this:

#[derive(Pod, Zeroable)]
#[repr(transparent)]
struct MyNewtype<T>([T; 2]);

However, the current derive system only allows for repr(packed) to be used with generics, despite the fact that, by definition, repr(transparent) contains no padding. This PR adds the option for repr(transparent) to be used in this way.

I also moved to checks on the actual implementation to the impl block rather than being static, so that MyNewtype can also be used in the non-Pod case. I've verified that this works for my use case.

@Lokathor Lokathor requested a review from fu5ha October 30, 2022 22:01
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

LGTM ✅

derive/src/lib.rs Outdated Show resolved Hide resolved
derive/src/lib.rs Outdated Show resolved Hide resolved
derive/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@fu5ha fu5ha left a comment

Choose a reason for hiding this comment

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

The idea seems good to me, will defer to @danielhenrymantilla on implementation review :)

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

The new add_trait_marker LGTM ✅; just note that there are a bunch of rustfmt-induced changes as well in that last commit

@notgull
Copy link
Contributor Author

notgull commented Nov 3, 2022

Whoops, I guess I just run rustfmt before commit by instinct nowadays.

@Lokathor
Copy link
Owner

Lokathor commented Nov 3, 2022

rustfmt is no big deal, i do the same thing.

@Lokathor Lokathor merged commit 518baf9 into Lokathor:main Nov 3, 2022
@notgull notgull deleted the transparent branch November 4, 2022 16:11
@Lokathor
Copy link
Owner

Lokathor commented Nov 5, 2022

Released bytemuck_derive-1.3.0

leod pushed a commit to leod/bytemuck that referenced this pull request Jun 3, 2023
…kathor#139)

* Enabled transparent generics

* Move trait checks to implementation block

* Replace add_trait_marker impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants