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

Impl bytemuck traits for GenericFamily #213

Merged

Conversation

waywardmonkeys
Copy link
Contributor

This can be used in the future as part of FFI work, but for now, the Contiguous trait is used to provide a MAX_VALUE used by GenericFamilyMap.

This is a backport of part of #209 where the separation between GenericFamily and GenericFamilyMap has grown to span crates.

The testing here borrows from (copies) what is done for similar testing of bytemuck traits in the color crate.

These crates already depend on bytemuck, so this isn't a new dependency. A decision is made here though to not use the derive feature as when we move this type to a vocabulary crate, we will want to avoid the derive feature there to avoid a dependency on proc macros.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Other than the incorrect test, this looks good to me.

Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

Could we feature flag the bytemuck impls? bytemuck is pretty small as a dependency. But it should also be easy to feature flag?

@waywardmonkeys
Copy link
Contributor Author

Could we feature flag the bytemuck impls? bytemuck is pretty small as a dependency. But it should also be easy to feature flag?

Probably not.

It is already used by the font stack and the reason that I used it here is to provide a better implementation of something for GenericFamilyMap.

I did an earlier version where I had a const and used that for GenericFamilyMap and used it in the Contiguous impl as well which let it be optional, but it didn't seem worth the hassle.

We'll also use this once we look at FFIs (hopefully).

@waywardmonkeys waywardmonkeys force-pushed the bytemuck-generic-family branch from 713ab68 to 53ceac3 Compare December 9, 2024 10:58
@nicoburns nicoburns dismissed their stale review December 9, 2024 11:02

Ok, I guess we can do unconditional then.

@nicoburns
Copy link
Contributor

I'm a little uncomfortable with those unsafe impls. Specifically, it seems like it would be very easy for the MIN and MAX constants to get out of sync. I think we ought consider checking those in CI with a macro at some point.

But they do seem to be correct now.

@waywardmonkeys
Copy link
Contributor Author

They are tested in the PR already.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 9, 2024

We should probably have a comment in GenericFamily about adding variants.

Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

Tests seems to work in both directions (MAX too high and MAX too low). Nice.

This can be used in the future as part of FFI work, but for now,
the `Contiguous` trait is used to provide a `MAX_VALUE` used by
`GenericFamilyMap`.

This is a backport of part of linebender#209 where the separation between
`GenericFamily` and `GenericFamilyMap` has grown to span crates.

The testing here borrows from (copies) what is done for similar
testing of `bytemuck` traits in the `color` crate.

These crates already depend on `bytemuck`, so this isn't a new
dependency. A decision is made here though to not use the `derive`
feature as when we move this type to a vocabulary crate, we will
want to avoid the `derive` feature there to avoid a dependency
on proc macros.
@waywardmonkeys waywardmonkeys force-pushed the bytemuck-generic-family branch from 53ceac3 to d9dbfc5 Compare December 9, 2024 11:40
@waywardmonkeys waywardmonkeys added this pull request to the merge queue Dec 9, 2024
Merged via the queue into linebender:main with commit 1d9d344 Dec 9, 2024
20 checks passed
@waywardmonkeys waywardmonkeys deleted the bytemuck-generic-family branch December 9, 2024 11:47
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.

3 participants