Custom types for the meta table #1244
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmyr
force-pushed
the
more-meta-impl
branch
2 times, most recently
from
122c333
to
c6096cf
Compare
This commit adds the code necessary to read and write the `meta` table, including its codegen definition, manual extras to handle the variable length data fields, and plumbing to surface it within `read-fonts` and `write-fonts`. Where possible, this commit borrows liberally from the implementation of other tables, especially `name` (for its handling of variable length data). This is an early implementation, and as such requires manual and automated testing. Additionally, there is an implementation of the FontWrite trait for &[u8] that may be redundant.
This adds types and the required impls that can represent the expected contents of the 'meta' table. This has to be handwritten, because the 'meta' table contains different payloads depending on the included tag for each record. I think having these as real types will provide a significantly better API though, and avoid us writing invalid data. The actual implementations for these types are very spartan; eventually I imagine having parsing & constructing code for the ScriptLangTag struct that ensures it conforms to the spec.
rsheeter
reviewed
Nov 15, 2024
write-fonts/src/tables/meta.rs
Outdated
| } | ||
| }; | ||
|
|
||
| let len: u32 = len.try_into().unwrap(); |
There was a problem hiding this comment.
I don't love the unwrap, is this common in write-fonts? Should file an issue to make write_into fallible and remove that?
There was a problem hiding this comment.
By design FontWrite is infallible; we do all the error checking in an earlier validation pass.
In generated code, we do add a check that an array's length can be represented by the type that stores the length, but I didn't bother hand-writing that here, given that any single font table's length must be representable in 32 bits or else its offset cannot be stored in the root table directory.
There was a problem hiding this comment.
Maybe we should comment as much and do an infallible production of len?
rsheeter
approved these changes
Nov 15, 2024
rsheeter
approved these changes
Nov 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is based on #1237; I think it makes sense to use custom types for the actual data in this table, but I wouldn't have been able to explain what to do without trying it first, so here we are.
(also this needs tests)