Drop odd-length flat dead byte; make --analyze score consistent - #11
Merged
Conversation
InnerLayer.split() padded self.data in place to make pairing work, but the flat (unsplit) solution emits self.data directly, so odd-length flat tables carried one unreachable trailing padding byte (and the reported cost, computed from the un-padded length, was one element short of the emission). Pad a local copy instead; self.data stays the original array, so the flat solution emits exactly len(data) elements. Split solutions are unchanged (pairing still uses the optimally-padded copy), so solution costs — and pick_solution's choices — are identical; only the flat array for odd-length tables loses its dead byte. Separately, `--analyze` computed the displayed Score with floor(log2) (bit_length()-1) while pick_solution uses exact log2, so the highlighted "Best solution" could disagree with the minimum-score row. Use exact log2 and show two decimals so the ranking is visibly consistent. Verified: 236 tests pass; a C+Rust round-trip fuzz over all Pareto solutions reports zero failures; regenerating HarfBuzz's tables yields byte-identical output in ~10s (the fix is a no-op for multi-level tables). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Two low-severity cleanups from the earlier soundness review.
1. Odd-length flat "dead byte"
InnerLayer.split()paddedself.datain place to make pairing work, butthe flat (unsplit) solution emits
self.datadirectly — so odd-length flattables carried one unreachable trailing padding byte, and the reported
.cost(computed from the un-padded length) was one element short of theemission.
Fix: pad a local copy instead.
self.datastays the original array, so theflat solution emits exactly
len(data)elements. Split solutions are unchanged(pairing still uses the optimally-padded copy), so solution costs — and
pick_solution's choices — are identical; only the flat array for odd-lengthtables loses its dead byte (one byte smaller, never larger). No runtime ops are
added to the generated code; generation cost is a negligible O(n) copy for odd
layers only.
2.
--analyzescore displayThe displayed
Scoreusedfloor(log2)(fullCost.bit_length() - 1) whilepick_solutionuses exactlog2, so the highlighted "Best solution" coulddisagree with the minimum-score row (e.g. two solutions whose fullCosts share a
floor(log2)bucket). Use exactlog2and show two decimals so the ranking isvisibly consistent.
Verification
pytest: 236 passing (adds tests: flat odd-length emits no padding byte,split()doesn't mutateself.data, flat odd-length round-trips in C+Rust,and
--analyzebest-row == min-score-row).make -f update-unicode-tables.make clean all) yields byte-identical output in ~10s (the flat fix is a no-opfor HarfBuzz's multi-level tables).
🤖 Generated with Claude Code