poc: resolve record offsets without passing FontData - #2085
Draft
cmyr wants to merge 1 commit into
Draft
Conversation
…f concept) Generated table getters for arrays of fixed-size, offset-bearing records now return ArrayOfRecordsWithOffsetData<T> instead of &[T]. This wrapper pairs the records with the enclosing table's data; its items are OffsetResolving<T>, which derefs to the record and carries generated no-argument getters that resolve each offset against the stored data: `rec.script()` instead of `rec.script(list.offset_data())`. This removes the caller's opportunity to pass the wrong table's data (see #1123). Design, briefly: - Only records whose offset getters are generated are wrapped; records using #[offset_getter] (e.g. NameRecord) resolve against something other than offset_data, so they keep &[T]. - The wrapper is an opaque vec-like type (len/get/iter) with as_slice() as an escape hatch; the old data-taking record getters remain, and the new methods delegate to them. Traversal and write-fonts from_obj use as_slice(), so their behavior is unchanged (otexplorer output verified byte-identical to main). - This is the fixed-size complement to positioned records (#2075): fixed-size records must stay byte-cast AnyBitPattern structs and so cannot hold the parent data themselves; here it is attached at the array level instead. Complications and things we may want to improve: - The inherent no-arg method on OffsetResolving<T> shadows the record's data-taking method through Deref; reaching the old form on a wrapped record requires an explicit .record(). Deliberate, but worth noting. - Call sites using binary_search or indexing need .as_slice()/.get(), and some function signatures changed to accept the wrapper or OffsetResolving directly (skrifa::charmap, skera's mark_array); where a plain &T is wanted, deref coercion mostly hides the difference. - Arrays of offset-bearing records *inside* records still yield &[T]: a fixed-size record has no data to seed the wrapper with. This is where this design meets #2075's positioned records, as a follow-up. - Naming (ArrayOfRecordsWithOffsetData / OffsetResolving) is provisional and a bit of a mouthful.
cmyr
marked this pull request as draft
August 27, 2026 16:41
Contributor
|
So this is the incremental approach rather than "break the world" and the changes look good, however: // before
pub fn script_records(&self) -> &'a [ScriptRecord];
// after
pub fn script_records(&self) -> ArrayOfRecordsWithOffsetData<'a, ScriptRecord>;will break Chrome (through fc-fontations in fontconfig which uses exactly this method) so we'd still need a migration path, which suggests a more comprehensive change to fix warts might have value? |
This file contains hidden or 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
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 a machine-assisted sketch of a way that we could bundle up offset data alongside arrays of records, so that each record in the array can resolve its own offsets.
This is like #2075, but for 'normal' records that we read by byte casting.
The names used here are intentionally too verbose.
Design, briefly:
Deref, and&OffsetResolving<T>coerces to&T, so most code that doesn't touch offsets is unaffected.Complications and things we may want to improve:
Below are the key ideas:
Core types:
Table getters for affected arrays change return type (16 accessors across layout, GPOS, cmap, COLR, BASE, feat, meta, bitmap):
And for each offset-bearing record, codegen emits no-argument resolvers that delegate to the existing data-taking getters (which remain, unchanged):
Records whose offsets do not resolve against
offset_data(those using#[offset_getter], e.g.NameRecord) are excluded and keep&[T].Iteration just loses the data argument (skrifa's autohint shaper, which threads three different parents'
offset_datathrough three nesting levels):for script in script_tags.iter().filter_map(|tag| { tag.and_then(|tag| script_list.index_for_tag(tag)) .and_then(|ix| script_list.script_records().get(ix as usize)) - .and_then(|rec| rec.script(script_list.offset_data()).ok()) + .and_then(|rec| rec.script().ok()) }) { for langsys in script .lang_sys_records() .iter() - .filter_map(|rec| rec.lang_sys(script.offset_data()).ok()) + .filter_map(|rec| rec.lang_sys().ok())Binary search uses the
as_slice()escape hatch, thenget()recovers aresolving record (COLR):