Behavior-preserving refactor — remove dead row-style state, share limit-gap math#218
Conversation
…et time Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the typesetting of stack math lists by removing the explicit listStyle and listCramped properties from MTMathStackConstruction and instead deriving them dynamically from the current script style and stack role. It also introduces helper methods upperLimitGapFor: and lowerLimitGapFor: in MTTypesetter to clean up limit gap calculations, and adds a unit test to verify the new behavior. The review feedback suggests adding parameter assertions to these new helper methods to ensure that the limit parameter is not nil before accessing its properties.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - (CGFloat) upperLimitGapFor:(MTDisplay*)limit | ||
| { | ||
| return MAX(_styleFont.mathTable.upperLimitGapMin, | ||
| _styleFont.mathTable.upperLimitBaselineRiseMin - limit.descent); | ||
| } |
There was a problem hiding this comment.
To ensure robust defensive programming and prevent potential issues if this helper is reused in the future, add a parameter assertion to verify that limit is not nil before accessing its properties.
- (CGFloat) upperLimitGapFor:(MTDisplay*)limit
{
NSParameterAssert(limit);
return MAX(_styleFont.mathTable.upperLimitGapMin,
_styleFont.mathTable.upperLimitBaselineRiseMin - limit.descent);
}There was a problem hiding this comment.
Declining. Both call sites guard with if (superScript) / if (subScript) immediately above (MTTypesetter.m:1740-1745), so nil can't reach the helper today. And even if it did, sending descent/ascent to a nil receiver returns 0 in Obj-C, so MAX(upperLimitGapMin, upperLimitBaselineRiseMin - 0) would yield a valid CGFloat — not a crash. NSParameterAssert is also a no-op in release builds (NS_BLOCK_ASSERTIONS), so it would only signal in debug. Helper is private to MTTypesetter; no future caller to defend against (YAGNI).
| - (CGFloat) lowerLimitGapFor:(MTDisplay*)limit | ||
| { | ||
| return MAX(_styleFont.mathTable.lowerLimitGapMin, | ||
| _styleFont.mathTable.lowerLimitBaselineDropMin - limit.ascent); | ||
| } |
There was a problem hiding this comment.
To ensure robust defensive programming and prevent potential issues if this helper is reused in the future, add a parameter assertion to verify that limit is not nil before accessing its properties.
- (CGFloat) lowerLimitGapFor:(MTDisplay*)limit
{
NSParameterAssert(limit);
return MAX(_styleFont.mathTable.lowerLimitGapMin,
_styleFont.mathTable.lowerLimitBaselineDropMin - limit.ascent);
}There was a problem hiding this comment.
Declining for the same reason as the upper-limit thread: caller guards with if (subScript) (MTTypesetter.m:1743), Obj-C nil-messaging would return a safe 0 anyway, and NSParameterAssert is a release-build no-op. Private helper, no speculative callers. YAGNI.
Code review — PR 1 of 2 (behavior-preserving refactor)Reviewed against the LLD ( VerdictLooks correct and ready to proceed. No Critical or Important issues found. Not approving here per request — flagging two Minor / nice-to-have items below. Item 2 —
|
Goal
Delete the unreachable
listStyle/listCrampedparse-time fields and derive the row style at typeset time; extract the operator-limit gap formula into shared helpers. No LaTeX behavior changes — the existing 268-test suite stays green. This isolates the pure refactor so the follow-up feature PR's diff is small and reviewable.This is PR 1 of 2 in the over/under stack-commands stack (
\overset,\underset,\stackrel,\stackbin).Design docs
docs/plans/2026-06-07-overset-underset-stackrel.mddocs/lld/2026-06-07-overset-underset-stackrel.mdCommits
[item 1]Remove dead row-style state; derive stack row style at typeset time[item 2]ExtractupperLimitGapFor:/lowerLimitGapFor:limit-gap helpersVerification
swift test— 268 tests, 0 failures.MTTypesetterTestlimit tests (80 tests).🤖 Generated with Claude Code