Skip to content

[codegen] Special-case CFF INDEX struct size#1728

Open
behdad wants to merge 1 commit intomainfrom
cff-index-size
Open

[codegen] Special-case CFF INDEX struct size#1728
behdad wants to merge 1 commit intomainfrom
cff-index-size

Conversation

@behdad
Copy link
Contributor

@behdad behdad commented Feb 4, 2026

I don't know if hacking codegen like this is kosher, but I think we need some code to this effect. CFF INDEXes are self-contained structs and know their own length. Please advise.

@behdad behdad requested review from cmyr and dfrg February 4, 2026 05:43
fn iter_field_validation_stmts(&self) -> impl Iterator<Item = TokenStream> + '_ {
self.fields.iter().map(Field::field_parse_validation_stmts)
let table_name = self.raw_name().to_string();
let is_index = table_name == "Index1" || table_name == "Index2";
Copy link
Member

Choose a reason for hiding this comment

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

This is a funny little case hm? In general we don't like to have this kind of code be actually defined in codegen. Currently we have special labels for the different special patterns for computing a count (such as the add_multiply, used elsewhere in Index1 and Index2 and so following that pattern we would have some annotation like, #[count(cff_index_data($offsets)], and maybe we would also introduce a type for the offsets themselves that would abstract the variable length so we could use a ComputedArray, which would simplify getting the last element... let me try putting that patch together as a first step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

thinking about this more, is this patch motivated by a specific concern? it is true that the raw data array here will potentially include extra bytes belonging to another table, but given that we know the start/end offsets of the actual objects, is there any risk of us misinterpreting the data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well one issue that current code is incorrect is that if count=0, we shouldn't expect offSize at all.

As for consuming the rest of the bytes, I noticed it, because I'm working on VARC table rendering, which uses Index2 in a couple of different ways. So I wasn't sure if the current code will work for me.

Copy link
Member

Choose a reason for hiding this comment

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

We could just mark the count field as a version and then set the others to min version of 1. Hacky but should work?

Copy link
Member

Choose a reason for hiding this comment

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

we have a general concept of a field being conditional on something, so that's fine: the annoying bit is just that it means off_size would become Option<u8>.

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