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

fontique: Remove unsafe in fontconfig cache #78

Merged

Conversation

waywardmonkeys
Copy link
Contributor

Instead of transmuting the CharSetLeaf to the underlying value and then doing the operations on that, use the operation as already exposed via CharSetLeaf::contains_byte.

Instead of transmuting the `CharSetLeaf` to the underlying value
and then doing the operations on that, use the operation as already
exposed via `CharSetLeaf::contains_byte`.
@waywardmonkeys
Copy link
Contributor Author

I'm unable to test this locally, so some care should be exercised.

In #77, updating to Rust 1.79 enabled a new clippy lint about how transmute calls are annotated and instead of doing what it suggested, I thought it would be better to get rid of the transmute entirely instead.

One option was to just call font.coverage.leaves.push(leaf.map) instead of transmuting, since the map has the right type and is pub, so no transmute would be required.

That felt a bit odd though to do and I thought maybe this approach would be better since it would use the code that already exists within fontconfig-cache-parser.

Is there an issue with this not inlining that code? (If so, could flag that method in a future release of fontconfig-cache-parser as #[inline] perhaps?)

Or, going by my mental track record of late after illness, I could just be entirely wrong here.

@waywardmonkeys
Copy link
Contributor Author

(Another option might have been to use even more code from fontconfig-cache-parser, but I don't know enough at that point about this and the requirements to be sure...)

Copy link
Contributor

@jneem jneem left a comment

Choose a reason for hiding this comment

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

Looks fine to me, although I'm not sure that I have approval authority.

Copy link
Collaborator

@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.

I can’t say why I didn’t structure the code this way originally but the change looks good to me. Thanks!

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Jun 16, 2024
Merged via the queue into linebender:main with commit e2102bf Jun 16, 2024
19 checks passed
@waywardmonkeys waywardmonkeys deleted the remove-usage-of-transmute branch June 16, 2024 01:50
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