Skip to content

Handle extension tables automatically in addfeatures - #1851

Draft
skef wants to merge 22 commits into
adobe-type-tools:developfrom
skef:feature/auto-overflow-handling
Draft

Handle extension tables automatically in addfeatures#1851
skef wants to merge 22 commits into
adobe-type-tools:developfrom
skef:feature/auto-overflow-handling

Conversation

@skef

@skef skef commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses #1849. Implements automatic subtable splitting and extension promotion so that addfeatures/makeotf can handle kerning overflow without requiring manual useExtension or subtable; directives in feature files.

Subtable splitting (internal overflow > 64K)

  • PairPos Format 2 (class kern): partitions Class1 classes into N groups when the class matrix exceeds 64K
  • PairPos Format 1 (specific pairs): greedy split by first-glyph groups

Extension promotion (offset overflow)

  • After all subtables are constructed, promotes the largest inline subtables to extension format (in decreasing size order) until the total inline section fits within the 64K offset budget
  • Honors explicitly-marked useExtension subtables first (they're already "out" of the budget)

Design principles

  • Respects explicit subtable; breaks and useExtension directives — automatic handling only supplements what the user specified
  • New -nao flag for addfeatures (also supported by makeotf) suppresses auto-overflow handling for users who need manual control
  • Emits info messages in verbose mode when auto-splitting or auto-promoting

Known limitations (draft)

  • Extension promotion offset recomputation for coverage/class table migration is incomplete — the coverage data is already built into the shared CoverageAndClass during subtable construction. A complete implementation must rebuild coverage for newly-promoted subtables.
  • The subtable splitting mechanism is complete and is the primary fix for the most common overflow scenario (large kern class tables).

Test plan

  • Full test suite passes (1621 tests)
  • Test with a real font that triggers Format 2 overflow (class1 × class2 > 32K)
  • Test with a font that triggers Format 1 overflow
  • Test -nao flag preserves current (fatal error) behavior
  • Verify verbose output shows info messages during auto-split/promote

🤖 Generated with Claude Code

skef and others added 2 commits July 7, 2026 13:44
@skef @claude
[Draft] Handle kerning overflow automatically in addfeatures
212b4fe 
Implements automatic subtable splitting and extension promotion to
handle GPOS kerning overflow without requiring manual intervention
in feature files. Addresses adobe-type-tools#1849.

Subtable splitting (internal overflow):
- PairPos Format 2: when the class matrix exceeds 64K, automatically
  partition Class1 classes into N groups and create separate subtables
- PairPos Format 1: when pair set data exceeds 64K, greedily split
  by first-glyph groups until each subtable fits

Extension promotion (offset overflow):
- After all subtables are constructed, check if the total inline
  section exceeds the 64K offset budget
- Promote the largest non-extension subtables to extension format
  (in decreasing order of size) until the remainder fits
- Honors explicitly-marked useExtension subtables first

Both mechanisms respect explicit subtable breaks and useExtension
directives already in the feature file. A new -nao flag suppresses
automatic overflow handling for users who want manual control.

NOTE: This is a draft. The extension promotion offset recomputation
has a known limitation around coverage/class table migration for
newly-promoted subtables. The subtable splitting is the primary
mechanism and is complete.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Fix extension promotion size calculation and validation
7c5f16c 
- Don't skip ext subtables when computing sizes from offset gaps;
  their 8-byte stubs are in the same offset space as regular subtables
- Add assertion validating that computed sizes sum back to the tracked
  offset.subtable total
- Report unresolvable overflow as sERROR (not sFATAL) so processing
  continues to the natural checkOverflow failure point

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 8, 2026
View reviewed changes
Comment thread c/addfeatures/hotconv/otl.cpp Fixed
skef and others added 4 commits July 8, 2026 01:33
@skef @claude
Store subtableSize at insertion time, simplify promotion logic
43f3501 
- Add subtableSize member to OTL::Subtable, computed in AddSubtable()
  as offset.subtable - sub->offset (no offset-gap arithmetic needed)
- Rewrite autoPromoteExtensions() to sort subtables by decreasing size
  directly, eliminating the SubSize struct and index-based lookups
- Size validation assertion no longer depends on vector ordering

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Add reference-counted coverage/class tracking for precise promotion
cf793d0 
Infrastructure for precise extension promotion decisions:

- Add refcount to CoverageRecord and ClassRecord
- Add coverageEndRC()/classEndRC() variants that increment refcount
  and return the record index
- Add activeCoverageSize()/activeClassSize() that count only entries
  with refcount > 0
- Add releaseCoverageRef()/releaseClassRef() that decrement refcount
  and return freed size when it hits 0
- Add cacCoverageRefs/cacClassRefs vectors to Subtable for tracking
  which shared cac entries each subtable references
- Update autoPromoteExtensions() to use releaseRef for precise savings
  calculation (subtable size + freed cac entries)

Subtable constructors still need to be wired up to use the RC variants
(coverageEndRC/classEndRC) to populate the ref vectors. Currently the
refcounts remain at 0 so the savings calculation is conservative
(cacFreed = 0). This is a no-op change in behavior until wired up.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Replace RC approach with private-cac-per-subtable architecture
09ec928 
Instead of reference counting shared cac entries, give every subtable
its own private CoverageAndClass during construction. This eliminates
the chicken-and-egg problem: promotion decisions don't depend on the
shared cac state, and the shared cac is built only after decisions
are finalized.

Architecture:
- Base Subtable constructor always allocates a private cac
- autoPromoteExtensions() uses calcMergedCacSize() to compute the
  hypothetical merged size for non-ext subtables (with dedup)
- Promotion loop: calculate merged cac, check fit, promote largest,
  repeat until total fits
- buildMergedCac() merges non-ext subtables' records into the shared
  cac with deduplication, updating offset fields via remapCacOffsets()
- Subtable types override remapCacOffsets() to fix their Coverage/
  ClassDef fields (virtual, default no-op)

Replaces the previous reference-counting approach which was more
complex and couldn't precisely account for dedup interactions.

NOTE: remapCacOffsets() overrides in GPOS/GSUB subtable types still
need to be implemented. This commit provides the infrastructure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Simplify: build merged cac directly in promotion loop
e202121 
Collapse calcMergedCacSize() and buildMergedCac() into a single
buildMergedCac() that returns the merged cac. The promotion loop
builds it each iteration and checks its size — when it fits, that's
the one we keep. No separate "estimate" vs "build for real" step.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 8, 2026
View reviewed changes
Comment thread c/addfeatures/hotconv/otl.cpp Fixed
skef and others added 2 commits July 8, 2026 02:43
@skef @claude
Implement call-sequence tracking and replay API for cac
840f90d 
Core infrastructure for the private-cac-per-subtable architecture:

- coverageEnd()/classEnd() now record the call sequence (which record
  index was returned for each call, preserving order and dedup)
- Add getCoverageOffset()/getClassOffset(): iterator-style replay that
  returns offsets in original call order, resolving through sharedCac
  if set (for non-ext subtables) or returning local offset (for ext)
- Add setSharedCac()/resetReplay() for controlling resolution
- Replace remapCacOffsets with setCacOffsets() virtual (no-op base) —
  subtable types override to call getCoverageOffset/getClassOffset
  mirroring their construction sequence
- buildMergedCac() now just replays records into merged pool and sets
  sharedCac pointers; called unconditionally from fillOTL()
- autoPromoteExtensions() uses temporary cacs for size checking only,
  does not set sharedCac — that's buildMergedCac's job

Remaining: setCacOffsets() overrides in each subtable type (GPOS/GSUB).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Add setCacOffsets() overrides for all GPOS/GSUB subtable types
fac03b9 
Each subtable type calls getCoverageOffset()/getClassOffset() in the
same order as its constructor called coverageEnd()/classEnd():

GPOS:
- SinglePos::Format1/Format2: Coverage
- PairPos::Format1: Coverage
- PairPos::Format2: Coverage, ClassDef1, ClassDef2
- ChainContextPos: backtracks[], inputGlyphs[], lookaheads[]
- CursivePos: Coverage
- MarkBasePos: MarkCoverage, BaseCoverage
- MarkLigaturePos: MarkCoverage, LigatureCoverage

GSUB:
- SingleSubst::Format1/Format2: Coverage
- MultipleSubst: Coverage
- AlternateSubst: Coverage
- LigatureSubst: Coverage
- ChainSubst: backtracks[], inputGlyphs[], lookaheads[]
- ReverseSubst: backtracks[], InputCoverage, lookaheads[]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 8, 2026
View reviewed changes
Comment thread c/addfeatures/hotconv/otl.cpp Fixed
skef and others added 4 commits July 8, 2026 13:35
@skef @claude
Remove subtable splitting from this branch (moved to separate branch)
ea4a111 
Restore original GPOS.cpp fill() and remove fillFormat1/fillFormat2
declarations. Subtable splitting will be on a branch based on this one.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Make cac a member variable on Subtable, not a shared_ptr
187c7db 
Since every subtable now owns its own CoverageAndClass, there's no
need for shared_ptr indirection. Change to a plain member:

- CoverageAndClass cac; (initialized in member initializer list)
- coverageSize()/classSize() delegate through sharedCac pointer when
  set, so write methods transparently get the merged total
- sharedCac is a raw non-owning pointer (set by buildMergedCac)
- Remove sub->cac = merged from buildMergedCac (no longer needed)
- All subtable code uses cac. (dot) notation
- OTL-level cac stays as shared_ptr (it's the merged pool)
- setCoverages() and classDefMake() take CoverageAndClass& not shared_ptr

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Use const_iterator for cac replay position
97467aa 
Replace size_t index counters with std::vector<uint16_t>::const_iterator
for the coverage/class call sequence replay. More idiomatic for
sequential access; no invalidation concern since vectors don't mutate
after construction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Remove isExt() branches from all subtable constructors
5b415f9 
All subtables are now constructed uniformly — no ext/non-ext
distinction at construction time. Every constructor just calls
incSubOffset(dataSize) regardless of useExtension.

Extension-related adjustments (Coverage += dataSize, extension.offset
assignment, incExtOffset) are deferred to a post-decision phase in
fillOTL(), after autoPromoteExtensions() determines which subtables
are actually extensions.

This resolves the "promoted subtables have wrong offsets" problem:
since no subtable gets ext-adjusted during construction, all of them
(explicit and promoted) get adjusted uniformly at the same later point.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef
skef force-pushed the feature/auto-overflow-handling branch from fb935f5 to a1ad178 Compare July 8, 2026 21:35
@skef @claude
Remove incSubOffset and dead Coverage assignments from constructors
6e4935c 
Constructors now just record cac data and store their size:
- incSubOffset(sz) → subtableSize = sz (direct assignment)
- Coverage = cac.coverageEnd() → cac.coverageEnd() (call for
  sequence recording, return value discarded)
- ClassDef = classDefMake(...) → classDefMake(...) (discarded)
- setCoverages pushes 0 placeholders (overwritten by setCacOffsets)
- Base constructor sets offset(0) (assigned in post-decision phase)
- AddSubtable accumulates offset.subtable from stored subtableSize

Values that were dead stores (overwritten by setCacOffsets or
adjustForExtension) are no longer assigned, making it clear that
offset fields are set in the post-decision phase, not at construction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef
skef force-pushed the feature/auto-overflow-handling branch from a1ad178 to 6e4935c Compare July 8, 2026 21:41
@skef @claude
Add post-decision offset assignment phase in fillOTL
a39bcb4 
Assign final subtable offsets after promotion decisions and cac merge:
- Non-ext: offset += subtableSize
- Ext: offset += extStubSize, extension.offset tracks ext section

Remove stale offset.subtable tracking from autoPromoteExtensions
(computed from scratch each iteration in the promotion loop).

NOTE: tests still failing - offset/type corruption under investigation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef
skef force-pushed the feature/auto-overflow-handling branch from bed9814 to b1555be Compare July 8, 2026 22:54
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 8, 2026
View reviewed changes
Comment thread c/addfeatures/hotconv/otl.cpp Fixed
@skef @claude
Add CoverageAndClass::mergeFrom() to share cac merge logic
8d89f57 
Extract the repeated "iterate records and register into target"
pattern into mergeFrom(). Used by both buildMergedCac() and the
temp cac size check in autoPromoteExtensions().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef
skef force-pushed the feature/auto-overflow-handling branch from b1555be to 8d89f57 Compare July 9, 2026 06:34
skef and others added 3 commits July 8, 2026 23:49
@skef @claude
Unify ext/non-ext offset adjustment in write() methods
0f0619b 
Call setCacOffsets() on extension subtables too (they resolve through
their private cac). Then unify the offset adjustment in all write()
methods: instead of guarding with if (!isExt()), compute adjust as
either subtableSize (ext: coverage follows data) or subOffset()-offset
(non-ext: distance to shared coverage section). The rest of the
calculation is identical for both paths.

This eliminates the need for a separate adjustForExtension() virtual.
Extension subtables now get correct Coverage/ClassDef offsets through
the same code path as non-extension ones.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Add test data for dedup-aware extension promotion
54b859b 
Test feature files exercising cac deduplication during promotion:

- cursive_range_a/b/c.fea: 60/70/80-rule lookups with unique
  coverages (different sizes, validate size-descending heuristic)
- test_dedup_promotion.fea: 3 unique + 2 shared + fillers, total
  overflows. Tests that promoting unique lookups frees coverage from
  merged cac, while shared lookups' coverage stays (other users)
- test_dedup_promotion_one_explicit.fea: same but unique_small is
  already useExtension (tests that explicit extensions reduce
  auto-promotions)

Test functions to exercise these will be added in a follow-up.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Add auto-extension promotion tests
4239322 
- test_auto_extension_promotion: verifies overflow font compiles with
  auto-promotion producing a valid font with extension lookups
- test_auto_extension_promotion_explicit_reduces: verifies explicit
  useExtension reduces number of auto-promotions needed
- test_auto_extension_nao_flag: verifies -nao preserves fatal overflow
- test_auto_extension_verbose_message: (xfail) verbose output check
- Add -nao to existing overflow tests so they continue testing the
  fatal error path
- Remove stray debug cerr print from GPOS CursivePos constructor

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef
skef force-pushed the feature/auto-overflow-handling branch from 4978e71 to 4239322 Compare July 9, 2026 08:12
@skef @claude
Remove all early overflow checks from constructors
55d2c92 
Early checkOverflow calls in constructors no longer check meaningful
values (offset.subtable is raw accumulated sizes, not true inline
total). Replace with a single correct check in fillOTL() after offset
assignment, gated by -nao flag.

- Remove 5 earlyCheck calls (GPOS LookupEnd, ChainContext; GSUB
  LookupEnd, ChainSubst, ReverseSubst)
- Remove 4 individual-size checks that were incorrectly rewritten
  from accumulated-total checks (CursivePos, MarkBasePos,
  MarkLigaturePos, LigatureSubst)
- Remove earlyCheck parameter from checkOverflow() (no longer needed)
- Add single overflow check in fillOTL() using correct computed total
  (offset.subtable + merged cac sizes), only fires with -nao
- Update test: overflow tests check for generic "offset overflow"
  message since per-type messages no longer exist

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef
skef force-pushed the feature/auto-overflow-handling branch from 697ab92 to 55d2c92 Compare July 9, 2026 08:47
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 9, 2026
View reviewed changes
Comment thread c/addfeatures/hotconv/otl.cpp Fixed
skef and others added 3 commits July 9, 2026 13:39
@skef @claude
Fix format specifier mismatch: %lx -> %x for LOffset (uint32_t)
276f03e 
CodeQL flagged that %lx expects unsigned long but LOffset is uint32_t.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Remove dead code: extOffset(), incExtOffset(), incSubOffset()
2e37d18 
These methods are no longer called — offset.subtable and
offset.extension are now computed directly in fillOTL()'s offset
assignment loop. Also remove the redundant offset.subtable
accumulation in AddSubtable() (reset and recomputed in fillOTL).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef @claude
Fix HOT_NO_AUTO_OVERFLOW bit collision with HOT_VERBOSE
3257b0e 
Both were (1 << 15), causing -V to also suppress auto-overflow
handling. Move HOT_NO_AUTO_OVERFLOW to bit 16.

Remove xfail from test_auto_extension_verbose_message (now passes).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@skef skef changed the title Handle kerning overflow automatically in addfeatures Handle extension tables automatically in addfeatures Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@github-advanced-security github-advanced-security[bot] github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@skef @github-advanced-security