\mathit companion font on MTFont - #270
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesMath italic companion font
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MTFont
participant CoreText
participant FontBundle
MTFont->>CoreText: Resolve verified italic PostScript font
CoreText-->>MTFont: Return matching CTFontRef or reject substitution
MTFont->>FontBundle: Load bundled Latin Modern italic fallback
FontBundle-->>MTFont: Return fallback CTFontRef
MTFont->>MTFont: Store and resize companion font
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kostub
left a comment
kostub
left a comment
There was a problem hiding this comment.
Reviewed the diff of #270 only (items 3-4), against LLD §2.8/§2.9/§3.1/§6/§7.
CoreFoundation memory management — checked, no defects found
I walked every path that creates, retains or releases a CTFontRef/CGFontRef/CGDataProviderRef, and also ran clang --analyze -fobjc-arc over MTFont.m (clean, exit 0).
- init, success.
MTCreateCompanionForMathFontreturns +1; assigned straight to the ivar (not through the setter), so the object holds exactly one reference;dealloc'sself.mathitCTFont = nilreleases it. Balanced. - init, early return at
if (!dict) { return nil; }._mathitCTFontis stillNULLat that point, and the setter'sif (_mathitCTFont != nil)guard makes thedeallocrelease a no-op. No over-release. - init, assert-fallback.
_mathitCTFont = CFRetain(_ctFont)puts +2 on the same object;deallocreleases through bothself.ctFont = nilandself.mathitCTFont = nil. Balanced — the ordering indeallocis safe becausesetCtFont:only nils_ctFont, it does not touch_mathitCTFont. copyFontWithSize:.CTFontCreateCopyWithAttributes+1 → setter retains (+2) →CFRelease(+1), held bycopyFont. Balanced, and the target is a fresh[[MTFont alloc] init]so the setter's release-old branch is not taken on aNULLivar.MTCreateBundledItalicFont.providerandcgFontare each released on both the success and the failure continuation; the threereturn NULLpoints leak nothing.MTCreateVerifiedFontWithPostScriptName.CFBridgingRelease(CTFontCopyPostScriptName(...))consumes the +1 name; the mismatch branch releasesfontbefore returningNULL.
I also empirically checked the one thing the LLD §6 re-entrancy note asserts but the test suite does not cover: whether CTFontCreateCopyWithAttributes preserves a face that was built from an unregistered CGFont (lmroman10-italic.otf is not installed on the system). testCopyFontWithSizePreservesMathitCompanion only exercises XITS, whose companion is a system-installed Times. I ran a scratch test over all eight rows: latinmodern-math and newcm-math both round-trip LMRoman10-Italic at the new size with identical glyph IDs for f/1/Γ. So the copy path is sound for the bundled face too — no finding, just closing the gap I suspected.
Behaviour matches §3.1's table row-for-row (all six by-name keys match the MTFontName* constant values), the fonts group is a folder reference in iosMath.xcodeproj so the new .otf needs no .pbxproj edit, and Package.swift's .copy("fonts") picks it up. Full suite: 455 tests, 0 failures.
Nit — two deletable lines
static CTFontRef MTCreateBundledItalicFont(CGFloat size) CF_RETURNS_RETAINED;
static CTFontRef MTCreateCompanionForMathFont(NSString* name, CGFloat size) CF_RETURNS_RETAINED;These two forward declarations exist only to carry the annotation — nothing calls either function before its definition. The annotation is not doing work: both names start with MTCreate, so the analyzer already applies the CF create rule by naming convention. I deleted both lines and re-ran clang --analyze: still clean, no new leak or over-release diagnostics. The file already relies on this — MTCreateVerifiedFontWithPostScriptName's own definition at line 26 carries no annotation either (only the MTFont+Internal.h declaration does), so as written the file is inconsistent about a marker that changes nothing. Four lines collapse to zero.
Same size of point, same place: the new test uses @import CoreText; where the neighbouring MTTypesetterTest.m:10 uses #import <CoreText/CoreText.h>. Both work; the #import form is what the rest of iosMathTests/ uses.
Nothing blocking.
Whole, unsubsetted Latin Modern Roman Italic face copied from TeX Live 2025. No .plist sidecar: it is drawn from as a plain CTFont, never consulted as a math table (LLD 2026-07-27 §2.8, §3.1). Already covered by the vendored GUST Font License. Not yet referenced by any code; resolved and carried by MTFont in the next item.
Every MTFont now resolves and carries a mathitCTFont at load: the two Latin Modern-family fonts (and any failed by-name lookup) fall back to the bundled lmroman10-italic.otf; the other six resolve a specific platform face by PostScript name, verified via MTCreateVerifiedFontWithPostScriptName to reject CoreText's silent substitution (LLD 2026-07-27 §2.9). copyFontWithSize: resizes the same face rather than re-resolving it. Nothing consumes mathitCTFont yet; routing lands in PR 3.
d4b2927 to
894f8de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
iosMathTests/MTFontManagerTest.m (1)
129-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProve the substitution branch.
Create the raw font with
CTFontCreateWithName, assert it is non-NULL, and assert thatCTFontCopyPostScriptNamediffers from the requested name before asserting thatMTCreateVerifiedFontWithPostScriptNamereturnsNULL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMathTests/MTFontManagerTest.m` around lines 129 - 133, Update testVerifiedFontCreationRejectsSubstitution to first create the requested font via CTFontCreateWithName, assert the raw font is non-NULL, and verify CTFontCopyPostScriptName differs from the requested name; then assert MTCreateVerifiedFontWithPostScriptName returns NULL while preserving proper CFRelease cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@iosMath/render/MTFont.m`:
- Around line 169-171: In the CTFontCreateCopyWithAttributes call for the
companion mathitCTFont, replace the size parameter with
CTFontGetSize(copyFont.ctFont) so that the companion font uses the same resolved
size as the primary font instead of preserving its original size when size is 0.
This ensures both fonts maintain equal sizes after copyFontWithSize is called.
In `@iosMathTests/MTFontManagerTest.m`:
- Around line 95-102: Guard the nullable results from fontWithName:size: and
related font-copy operations in the test methods: assert font, copy, and
companion, then return or continue before any Core Text call when a value is
nil. Update the affected checks around mathitCTFont and the corresponding lines
to prevent NULL from reaching Core Text APIs while preserving the existing
assertions for valid objects.
---
Nitpick comments:
In `@iosMathTests/MTFontManagerTest.m`:
- Around line 129-133: Update testVerifiedFontCreationRejectsSubstitution to
first create the requested font via CTFontCreateWithName, assert the raw font is
non-NULL, and verify CTFontCopyPostScriptName differs from the requested name;
then assert MTCreateVerifiedFontWithPostScriptName returns NULL while preserving
proper CFRelease cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd1cf13e-533b-4514-af3d-0f195a4b03af
⛔ Files ignored due to path filters (1)
iosMath/fonts/lmroman10-italic.otfis excluded by!**/*.otf
📒 Files selected for processing (3)
iosMath/render/MTFont.miosMath/render/internal/MTFont+Internal.hiosMathTests/MTFontManagerTest.m
copyFontWithSize: passed its `size` argument to both font constructors, but they read 0 differently: CTFontCreateWithGraphicsFont maps it to 12pt while CTFontCreateCopyWithAttributes keeps the source size. So copyFontWithSize:0 — reachable from the public fontWithName:size: — produced a 12pt math font with a 20pt \mathit companion. Build the companion from the primary's resolved size instead. Drop the two CF_RETURNS_RETAINED forward declarations: nothing calls either function before its definition, and the analyzer already applies the CF create rule to an MTCreate* name (verified — it still reports an injected leak through MTCreateBundledItalicFont without them). Test file switches to #import <CoreText/CoreText.h>, matching the rest of iosMathTests/, and testVerifiedFontCreationRejectsSubstitution now pins that CoreText substitutes rather than returning NULL, which is the premise the whole verify-by-name path exists for. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
1 participant
Plan:
docs/plans/2026-08-04-mathit-text-italic-routing.md(PR 2 of 3, items 3-4)LLD:
docs/lld/2026-07-27-mathit-text-italic-routing.mdGoal
Every
MTFontcarries amathitCTFont— the LLD §3.1 companion table made total over the eight bundled math fonts: bundled Latin Modern Roman Italic for the two Latin Modern-family fonts and as the terminal fallback, name-verified platform faces for the other six.Nothing consumes it yet; routing lands in PR 3.
Two details worth review attention:
MTFontcache rather than a second cache inMTFontManager— no(name, size)coherence problem and noMTFont.nameaccessor needed (LLD §3.3).CTFontCreateWithNamesubstitutes silently rather than failing (asking for a missing face returned Helvetica in the LLD §2.9 probe), so by-name rows create, compareCTFontCopyPostScriptName, and reject on mismatch — falling through to the bundled face rather than drawing a wrong one.The bundled
.otfships whole and unsubsetted with no.plistsidecar: it is drawn from, never used as a math table. The GUST Font License already covers it and is already vendored.Commits
[item 3] Bundle lmroman10-italic.otf companion face[item 4] Resolve \mathit companion font on MTFontFull
swift testgreen (455 tests).Stack
Summary by CodeRabbit
New Features
Bug Fixes