[behdad]

Explicit name overrides can now replace or insert a naming record, or remove it with None or an empty string. Plan::override_name_table follows HarfBuzz's override semantics: select source records first, apply removals and replacements, then insert nonempty overrides even when normal name, language, or platform filters exclude them.

Macintosh overrides require ASCII; other platforms encode Rust strings as UTF-16BE. Rejected encoding or length requests preserve the previous override. Dropped and passthrough name tables retain precedence, and serializer allocation accounts for inserted text.

This replaces #2292 with a clean commit directly on current main, without its historical stack-propagation merges.

Validation includes a saved naming-table reference from HarfBuzz d659591a6 covering surrogate pairs, embedded NULs, repeated requests, insertion past filters, and removals. Additional checks cover rejected requests, a 65,534-byte encoded name, and dropped/passthrough tables.

Tests:

• cargo +1.89 fmt --all --check
• cargo +1.89 clippy -p skera --all-features --all-targets -- -D warnings
• cargo +1.89 test -p skera --all-features

[qxliu76]

manual review comments:

1. could replace BTreeSet with HashSet for better performance
2. No need to build retained set if overrides is empty
3. We could pre-allocate records with Vec::with_capacity()
4. based on repacker perf tuning experience, BTreeMap is slow, maybe a FastHashMap is sufficient for plan.name_table_overrides?

below're comments from AI review:

Nothing that matters in practice. A name table typically holds tens to a few hundred records, so subsetting it takes microseconds. Still, a few things could be tighter, listed from most
to least worthwhile:

1. retained set is built on every run (worth fixing)

In name.rs:30-61, every retained record's key goes into a BTreeSet. That happens on every subset, even when there are no overrides, which is the common case. Only keys that actually
matched an override need tracking, and you already know which ones they are from the get() lookup:

let mut overridden = BTreeSet::new(); // never allocates if there are no overrides
...
let bytes = if let Some(o) = plan.name_table_overrides.get(&key) {
    overridden.insert(key);
    if o.is_empty() { continue; }
    o.as_slice()
} else { ... };
records.push((key, bytes)); // no per-record insert into the set
...
for (&key, bytes) in &plan.name_table_overrides {
    if !bytes.is_empty() && !overridden.contains(&key) {
        records.push((key, bytes.as_slice()));
    }
}

The output is the same: a key is "retained" exactly when some record that passed the filters looked it up. The set shrinks from one entry per retained record to at most one per override,
and it costs nothing when there are no overrides.

2. Small allocation and loop details

• records grows without a capacity hint. Use Vec::with_capacity(self.count() as usize + plan.name_table_overrides.len()).
• self.storage_offset() is re-read for every record (name.rs:56). Read it once before the loop.
• No early exit on empty overrides. With an empty map, the per-record name_table_overrides.get() and the insertion loop could be skipped entirely. Both are cheap on an empty BTreeMap,
though.

3. Each record is written as six separate fields

serialize_name_records calls s.embed(u16) six times per record, and each call does its own bounds check and error mapping. Building the 12-byte record in a [u8; 12] and writing it with
one embed_bytes call would mean one check per record. The same applies to the 3-field header. The code before this commit had the same pattern, so this is not a regression.

4. override_name_table encoding (negligible)

In lib.rs:476-478, encode_utf16().flat_map(u16::to_be_bytes).collect() starts from a low size estimate, so the vector reallocates a couple of times. Vec::with_capacity(text.len() * 2)
followed by extend avoids that; UTF-8 length × 2 is an upper bound on the UTF-16BE size. This runs once per API call, so it doesn't matter in practice.

[behdad]

Addressed in a02613f:

• changed Plan::name_table_overrides to FastHashMap
• track only keys that actually matched overrides, using a pre-sized fixed-state HashSet
• skip constructing the set and skip override lookups entirely when no overrides exist
• pre-allocate the output record vector and cache storage_offset
• pre-allocate UTF-16BE encoding storage
• serialize the fixed name header/records with one bounds-checked byte write each

Output remains deterministic because records are sorted before serialization. cargo test -p skera --lib --tests --all-features passes (321 unit tests plus all integration suites).

[qxliu76]

Remaining nits

1. UTF-16 buffer over-reserves (lib.rs:478-482). Vec::with_capacity(text.len() * 2) is exact for ASCII but too big otherwise. Per character it reserves 2× the needed space for "é" or
   emoji and 3× for CJK. These vectors live as long as the Plan and are never shrunk. Oversized input also allocates 2× its size before being rejected. Computing the exact size first fixes
   both:
   let len = text.chars().map(char::len_utf16).sum::() * 2;
   if len > u16::MAX as usize { return Err(...); }
   let mut bytes = Vec::with_capacity(len);
   The Mac path could likewise check text.len() before to_vec().
2. records capacity (count + overrides.len()) is sized before filtering. With the default name IDs 0–6 and English only, it often reserves 3–4× more than it uses. That's harmless: it's
   short-lived, and count is already checked against the table's real size.
3. Style:
   • overridden.as_mut().unwrap() relies on an invariant that isn't obvious. A combined if let (Some(set), Some(bytes)) = (overridden.as_mut(), overrides.get(&key)) would remove the
   unwrap.

non-performance items

• The test compares records but not the raw bytes; it could assert byte equality with name.bin.
• The README has the unrelated --no-bidi-closure line.
• The HarfBuzz link in name.rs:18-19 points at an old commit.

[behdad]

Addressed the remaining feedback in 92cb532:

• Compute the exact UTF-16BE buffer size and reject oversized overrides before allocating; Macintosh overrides also check length before copying. UTF-16 counting stops once the encoded limit is exceeded.
• Remove the overridden.as_mut().unwrap() while preserving the empty-overrides path that skips lookups.
• Assert byte-for-byte equality with the HarfBuzz name.bin reference, alongside the record comparisons.
• Expand length-limit coverage for ASCII, accented text, CJK, surrogate pairs, and oversized Macintosh input; rejected overrides preserve the previous request.
• Remove the unrelated --no-bidi-closure README line and update the HarfBuzz source link to the commit used for the reference fixture.

Kept the existing record-vector capacity hint, as the review notes that its temporary over-allocation is harmless.

Validation passed:

• cargo +1.89 fmt --all --check
• cargo +1.89 clippy -p skera --all-features --all-targets -- -D warnings
• cargo +1.89 test -p skera --all-features

[qxliu76]

1. Unexplained bounds check (name.rs L28-30): data.get(self.name_record_byte_range()).ok_or(..)?; throws away its result. It's only there because name_record() quietly returns an empty slice when the range is out of bounds. Add
   a comment, or compare self.name_record().len() against self.count().

2. The Option wrapper is unnecessary (L33-38, L56-64). FastHashSet::default() doesn't allocate, and get on an empty map returns early without hashing. A simpler version is just as fast:
   let bytes = match overrides.get(&key) {
   Some(o) => {
   overridden.insert(key);
   if o.is_empty() { continue; }
   o.as_slice()
   }
   None if record.length() == 0 => &[],
   None => { /* slice from string storage */ }
   };

3. Hand-built header and record bytes (L88-90, L105-116): the byte arrays are harder to read than the old s.embed(u16) calls, and the speed gain is negligible.
   • Clippy (1.98) flags chunks_exact_mut(2) with chunks_exact_to_as_chunks. Its suggested fix, as_chunks_mut, needs Rust 1.88, but the minimum supported version is 1.85. Going back to embed avoids the warning.
   • The old comment "10 is the position of offset field" was dropped, leaving s.length() + 10 unexplained. Restore it or write NAME_RECORD_SIZE - 2.

4. Type aliases: FastHashSet is defined locally in name.rs, while FastHashMap lives in lib.rs L102. Put both together. Also:
   • RecordKey spells out read_fonts::types::NameId instead of importing it.
   • Its field order matters for sorting; a one-line comment would say so.

5. override_name_table checks platform_id == 1 three times. Use one if mac { ascii check; to_vec } else { encode } followed by a single length check. Consider a named constant, or cmap::PlatformId::Macintosh, instead of the
   bare 1.

6. Error variant: InvalidNameOverride(String) only ever carries fixed messages, so &'static str would do.

7. Tests: name_overrides_test.rs uses both Tag::new(b"name") and Name::TAG, and its Key type duplicates RecordKey. Both are minor.

[behdad]

Addressed in c390564:

• Explain the explicit bounds check before name_record() so malformed record ranges cannot silently turn into an empty table.
• Replace Option<FastHashSet> with an initially empty set and a direct override lookup/match. The set allocates only when an override matches.
• Restore scalar s.embed() calls for the header and records, and express the string-offset position as NAME_RECORD_SIZE - 2.
• Put FastHashSet beside FastHashMap, import NameId, and document the sort order of RecordKey.
• Use one platform branch with PlatformId::Macintosh. Both encoding paths share a length-validation closure, preserving the earlier request to reject oversized names before allocating.
• Change InvalidNameOverride to carry &'static str.
• Use Name::TAG consistently in the integration test. Its local key tuple remains intentionally independent of the internal alias, avoiding a public API addition solely for testing.

Validation passed:

• cargo +1.85 check -p skera --all-features --locked
• cargo +1.89 fmt --all --check
• cargo +1.89 clippy -p skera --all-features --all-targets -- -D warnings
• cargo +1.89 test -p skera --all-features
• Clippy 1.98's reported lint specifically: cargo +nightly clippy -p skera --all-features --all-targets --no-deps -- -A warnings -D clippy::chunks_exact_to_as_chunks

The unrestricted Clippy 1.98 run still reports unrelated existing warnings in font-types/src/fixed.rs, skera/src/cff/charstring.rs, skera/src/graph.rs, and skera/src/hvar.rs.

[qxliu76]

Still open (all small)

1. estimate_subset_table_size still has the bare + 12 and no comment.
   • Use NAME_RECORD_SIZE; it's pub(crate) or can be moved up.
   • Add a note that the bump is needed for correctness: the retry loop gives up at table_len * 256, so a large override on a tiny name table would otherwise fail. HarfBuzz has the same limit and the same latent bug.
2. The checked_len closure in override_name_table works, but the Macintosh branch throws its result away (checked_len(text.len())?;). It's clearer to compute len once per branch and compare it to the limit in one place.
3. Optional speed-up: the UTF-16 path still walks the encoding twice. If text.len() <= u16::MAX as usize / 2, the encoded name always fits, so counting can be skipped. Names are short, so this barely matters.
4. Readability nit: for value in [0, count, storage_offset] would read better with let format = 0u16;.
5. Commit history:
   • Three of the four commits are review fixups; consider squashing them into 30aa4e5 before merging.
   • That also removes the README --no-bidi-closure line that 30aa4e5 adds and 92cb532 deletes again.

[behdad]

Addressed in ac1b977, with the PR squashed into a single commit as suggested.

• Use NAME_RECORD_SIZE in estimate_subset_table_size and explain why the override allowance is required for correctness: retries stop at table_len * 256, so a large override on a tiny source table can otherwise fail.
• Compute the encoded length in each platform branch, then validate it once before allocating. Removed the checked_len closure.
• Name the header's format field explicitly.
• Add an end-to-end regression test for a 65,534-byte override on a six-byte empty name table. Confirmed it fails with SubsetTableError(name) when the override allowance is disabled.

Kept exact UTF-16 sizing rather than the optional short-string shortcut, preserving the earlier fix for over-allocation.

Validation passed:

• cargo +1.89 fmt --all --check
• cargo +1.85 check -p skera --all-features --locked --offline --target-dir target/msrv
• cargo +1.89 clippy -p skera --all-features --all-targets --locked --offline -- -D warnings
• cargo +1.89 test -p skera --all-features --locked --offline (430 passed; 8 ignored by default)

[qxliu76]

Remaining items (minor)

1. Import style: use name::NAME_RECORD_SIZE; (lib.rs L49) is the only name:: item imported at the top. The same file writes name::RecordKey inline, so writing name::NAME_RECORD_SIZE at its single use would be more consistent.
2. Possible speed-up: the UTF-16 path still walks the encoding twice. When text.len() <= u16::MAX as usize / 2, the encoded name always fits and the counting pass can be skipped.
3. Commit message: the body is accurate. It could also mention two things that differ from HarfBuzz, so readers don't treat them as regressions: invalid overrides are rejected up front, and the name buffer estimate is bumped
   for large overrides.

[behdad]

Addressed in f6812ad, keeping the PR as a single commit.

1. Removed the top-level constant import and use name::NAME_RECORD_SIZE at its single call site, matching name::RecordKey.
2. Added the short-input UTF-16 fast path: when text.len() <= u16::MAX as usize / 2, twice the UTF-8 length is a safe capacity bound, so encoding skips the counting pass. The buffer is trimmed before storage to preserve the earlier fix against retained over-allocation. Longer inputs still use bounded counting and reject oversized names before allocating. Added end-to-end coverage for accented text, CJK, and emoji at 32,767 and 32,768 UTF-8 bytes; existing ASCII length-limit and tiny-table cases also pass.
3. Expanded the commit message to explicitly identify up-front validation and the initial-buffer allowance for large overrides as intentional differences from HarfBuzz, including why the retry limit requires the allowance.

An optimized local encoder-only microbenchmark favored the bound-and-trim approach over exact counting for five representative short names; these are not whole-font timings.

Validation passed:

• cargo +1.89 test -p skera --all-features --locked --offline (431 passed; 8 ignored by default)
• CARGO_INCREMENTAL=0 cargo +1.89 clippy -p skera --all-features --all-targets --locked --offline -- -D warnings
• cargo +1.89 fmt --all --check
• cargo +1.85 check -p skera --all-features --locked --offline --target-dir target/msrv

[qxliu76]

my agent says: ready to merge as is!

[behdad]

Yoohoo!