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

Adopt bytemuck crate #814

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Adopt bytemuck crate #814

merged 4 commits into from
Mar 20, 2024

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Mar 5, 2024

This replaces our internal FromBytes trait with bytemuck::AnyBitPattern.

I'm not sure if we're ready to pull the trigger on this yet, but I wanted to at least get a patch ready and see if it presented any unexpected problems. It did not.

  • This lets us remove the vast majority of unsafe from read-fonts. The only remaining unsafe here is trivial, and easily replaced with an unwrap:
    if pstring.is_ascii() {
    // SAFETY: `pstring` must be valid UTF-8, which is known to be the
    // case since ASCII is a subset of UTF-8 and `is_ascii()` is true.
    Ok(PString(unsafe { std::str::from_utf8_unchecked(pstring) }))
  • By using the derive macro provided by bytemuck, we can be generally confident that we will not end up inadvertently generate unsound trait impls.
  • This requires us to add two bits of unsafe impl trait to font-types, but these are localized and easy to reason about.
  • overall I think that this is a good tradeoff, and I am happy to not be responsible for maintaining any fiddly unsafe here.
  • see Consider using bytemuck as replacement for read_fonts::FromBytes #808

@drott
Copy link
Contributor

drott commented Mar 11, 2024

Please see #808 (comment) - ideally let's hold landing this at least as a mandatory dependency until Chromium adds bytemuck.

@rsheeter
Copy link
Collaborator

Please rig for bytemuck 1.13.1

cmyr and others added 3 commits March 18, 2024 14:22
This has one downside; we need to use unsafe to manually implement
AnyBitPattern for BigEndian<T>, because derive is not smart enough to
figure out that the type does not actually contain an instance of `T`
itself.
This removes our internal `FromBytes` trait with the `AnyBitPattern`
trait from the `bytemuck` crate. This removes the vast majority of our
unsafe (and our only tricky unsafe) and is an overall simplification.
* remove unsafe from skrifa...

and make both read-fonts and skrifa #![forbid(unsafe_code)]

* fix no_std

* remove unnecessary safety comment

* just unwrap from_utf8() in PString::read()

since we already check if is_ascii()
@cmyr cmyr marked this pull request as ready for review March 18, 2024 18:36
This is the most recent version which has undergone internal safety
review.
Copy link
Member

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

🚢

@cmyr cmyr merged commit dd494eb into main Mar 20, 2024
9 checks passed
@cmyr cmyr deleted the bytemuck-check branch March 20, 2024 15:28
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.

4 participants