Skip to content

Add support for bytemuck#336

Merged
KodrAus merged 3 commits intobitflags:mainfrom
KodrAus:feat/bytemuck
Apr 17, 2023
Merged

Add support for bytemuck#336
KodrAus merged 3 commits intobitflags:mainfrom
KodrAus:feat/bytemuck

Conversation

@KodrAus
Copy link
Member

@KodrAus KodrAus commented Apr 11, 2023

Closes #311

I've also added some docs on how to support external libraries here for future contributors. Since the bytemuck impls themselves are unsafe, I've added a note to where we generate our internal flags type warning that its ABI can't be changed.

@KodrAus
Copy link
Member Author

KodrAus commented Apr 11, 2023

cc @MarijnS95 do these look right to you? I haven't used bytemuck myself.

pub use core;

#[cfg(feature = "serde")]
pub use serde;
Copy link
Member Author

@KodrAus KodrAus Apr 11, 2023

Choose a reason for hiding this comment

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

These have moved into the external module alongside their supporting code

) => {
$(#[$outer])*
$vis struct $BitFlags(<Self as $crate::__private::PublicFlags>::Internal);
$vis struct $BitFlags(<$BitFlags as $crate::__private::PublicFlags>::Internal);
Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting a build error with Self being unknown here. Not sure if it was bytemuck's derives or a change in rustc but it was easily fixed.

@KodrAus
Copy link
Member Author

KodrAus commented Apr 17, 2023

cc @Lokathor 👋

I just wanted to make sure I've got these Pod and Zeroable impls right 🙂 In bitflags, we can automatically implement them on an internal type that looks like:

#[repr(transparent)]
struct FlagsField<T> {
    bits: T
}

where T may be a numeric type, like i32, usize etc. We may allow you to specify this type as something you've invented at some point in the future though. The type will always be a #[repr(transparent)] struct with a single field though.

The impls I've come up with look like this. Does that look right to you? Is there anything else we need to guarantee to make sure these impls are safe to do?

@Lokathor
Copy link

yep, impl<T> Pod for ReprTransparentIntNewtype<T> where T: Pod is basically exactly what I was expecting to see before I clicked, and then it's what I saw.

@KodrAus
Copy link
Member Author

KodrAus commented Apr 17, 2023

Awesome, thanks for checking them out! 🙇 I'll get this rolling through now.

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.

Support bytemuck

2 participants