-
Notifications
You must be signed in to change notification settings - Fork 24
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
avar2 support #1120
avar2 support #1120
Conversation
Took a quick skim and only saw minor nits but I think we want @cmyr to approve given the codegen changes. |
069fccd
to
3982ed7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, this fell through the cracks while I was off.
I'm going to just focus on the codegen changes for now, since that's really the meat of this PR; once we have the codegen right then everything else is pretty straight-forward.
avar2 introduces a pattern we have not seen in a previous table, which is a sequence of variable-length items which is not the last item in a table.
One of the major considerations for codegen is to try, whenever possible, to reduce the number of special cases. This makes it easier for contributors, but it also is important for reducing as much as possible the complexity of the codegen code, since it can be particularly difficult to reason about.
Given that goal, I think there is a slightly more aggressive change we can make to how VarLenArray
works that will be an overall improvement, and also support your case.
design proposal
I think you're on the right track here, but after looking at how VarLenArray
is used elsewhere, I think we should avoid having two different paths for 'counted' and 'uncounted', and just require that VarLenArray
is always provided with a count.
- for avar/gvar/fvar this will be fairly trivial; we'll need to define a new count helper that extracts the count from the
TupleVariationCount
field. - for
post
we will need a slightly more involved count helper that looks at theglyphNameIndex
array and determines how many strings we're storing by looking at the largest referenced index.
Then, instead of adding a method to the specific type we should make use of the existing VarSize
trait, which must already be implemented for any type used with VarLenArray
. It looks like we can handle this pretty easily with a single inherit method on the trait, that just calls the existing read_len_at
method, which would look something like,
pub trait VarSize {
/// The type of the first (length) field of the item.
///
/// When reading this type, we will read this value first, and use it to
/// determine the total length.
type Size: Scalar + Into<u32>;
#[doc(hidden)]
fn read_len_at(data: FontData, pos: usize) -> Option<usize> {
let asu32 = data.read_at::<Self::Size>(pos).ok()?.into();
(asu32 as usize).checked_add(Self::Size::RAW_BYTE_LEN)
}
/// Determine the total length required to store `count` items of `Self` in
/// `data` starting from `start`.
#[doc(hidden)]
fn total_len_for_count(data: FontData, start: usize, count: usize) -> Result<usize, ReadError> {
(0..count).try_fold(start, |current_pos, _i| {
Self::read_len_at(data, current_pos)
.and_then(|i_len| current_pos.checked_add(i_len))
.ok_or(ReadError::OutOfBounds)
})
}
}
(this impl is untested, but gives the general idea)
Alongside this change, it would be good if we also had specific codegen tests for this logic. This would involve updating resources/codegen_inputs/test_offsets_arrays.rs
to add a table that uses VarLenArray
, and then writing some tests in read-fonts/src/codegen_test.rs
that construct the test table under various conditions and ensure that everything is sane.
My strong preference is to do these codegen changes on their own, as a separate PR; that can be reviewed and merged in isolation, and then we can have a much simpler avar2 PR on top of it.
If you're interested in writing this patch you are very welcome to, otherwise I can find time to do it next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay this all looks sane, but I have a few little questions & thoughts inline, but big picture I think this is basically mergeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks! @simoncozens ping me to confirm there's nothing else to do here and I'll merge.
This has required messing about with the codegen to allow variable length subtables in the middle of a table - see commit message of first commit for more details.