Skip to content

Behavior-preserving refactor — remove dead row-style state, share limit-gap math#218

Merged
kostub merged 2 commits into
masterfrom
feature/overset-pr1
Jun 7, 2026
Merged

Behavior-preserving refactor — remove dead row-style state, share limit-gap math#218
kostub merged 2 commits into
masterfrom
feature/overset-pr1

Conversation

@kostub
Copy link
Copy Markdown
Owner

@kostub kostub commented Jun 7, 2026

Goal

Delete the unreachable listStyle/listCramped parse-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

  • Plan: docs/plans/2026-06-07-overset-underset-stackrel.md
  • LLD: docs/lld/2026-06-07-overset-underset-stackrel.md

Commits

  • [item 1] Remove dead row-style state; derive stack row style at typeset time
  • [item 2] Extract upperLimitGapFor:/lowerLimitGapFor: limit-gap helpers

Verification

  • swift test — 268 tests, 0 failures.
  • Per-commit test verification: both commits independently green.
  • Item 2 is a behavior-preserving extraction guarded by the existing MTTypesetterTest limit tests (80 tests).

🤖 Generated with Claude Code

kostub and others added 2 commits June 7, 2026 23:29
…et time

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1710 to +1714
- (CGFloat) upperLimitGapFor:(MTDisplay*)limit
{
return MAX(_styleFont.mathTable.upperLimitGapMin,
_styleFont.mathTable.upperLimitBaselineRiseMin - limit.descent);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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);
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +1716 to +1720
- (CGFloat) lowerLimitGapFor:(MTDisplay*)limit
{
return MAX(_styleFont.mathTable.lowerLimitGapMin,
_styleFont.mathTable.lowerLimitBaselineDropMin - limit.ascent);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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);
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kostub
Copy link
Copy Markdown
Owner Author

kostub commented Jun 7, 2026

Code review — PR 1 of 2 (behavior-preserving refactor)

Reviewed against the LLD (docs/lld/2026-06-07-overset-underset-stackrel.md) and plan (items 1–2). I checked out the PR tip (25206e9) in a clean worktree and ran the full suite: 268 tests, 0 failures, including all 80 MTTypesetterTest cases (the item-2 behavior guard) and the new testStackMathListRowRendersAtScriptStyle.

Verdict

Looks 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 — upperLimitGapFor: / lowerLimitGapFor: extraction is byte-identical

The extracted helpers preserve the exact computation from addLimitsToDisplay::

  • Same metrics source _styleFont.mathTable.{upper,lower}LimitGapMin / …BaselineRiseMin / …BaselineDropMin (not _font).
  • limit.descent / limit.ascent are bound to the same superScript / subScript displays as before.
  • MAX(...) argument order unchanged.

I grepped for any other inline copies of this formula — there are none, so the extraction has a single home and no caller diverges. The \sum\limits limit tests pass unchanged, confirming the byte-identical claim.

Public-symbol removal is safe

Verified by grep across iosMath/: in the PR-1 tree, +mathListWithList: is only ever called from MTMathStack finalized (re-wrapping an already-existing MathList construction). Every stackCommands entry uses extensibleWithGlyph:, so no LaTeX command can produce a kMTMathStackConstructionMathList. The removed listStyle / listCramped were therefore genuinely dead state, and dropping them changes no LaTeX behavior. This matches the "LaTeX is the compatibility contract" project rule and the LLD §2 public-API note. The removed style:cramped: overload shipped in 2.1.0–2.3.0, so this is an intentional source-break of an unreachable Obj-C API, documented in LLD R4.

Typeset-time style derivation in buildStackConstruction:

style:self.scriptStyle + cramped:(role == kMTStackRoleOver ? self.superScriptCramped : self.subscriptCramped) correctly mirrors the script/limit rendering rules used elsewhere in the file (makeScripts:, addLimitsToDisplay:): the over-row follows the superscript rule (cramped iff _cramped), the under-row follows the subscript rule (always cramped). _font is used as the font, consistent with the other createLineForMathList: call sites. The MTStackRole enum + role: plumbing through makeStack: is wired correctly (kMTStackRoleOver / kMTStackRoleUnder).

Minor / optional

  1. Helper nil-safety (echoing the Gemini note, with a caveat). upperLimitGapFor: / lowerLimitGapFor: dereference limit.descent / limit.ascent. All current callers (addLimitsToDisplay:) guard with if (superScript) / if (subScript), so nil can't reach them today. In PR 2's makeStack: the LLD calls these only inside the overDisp / underDisp non-nil branches, so still safe. An NSParameterAssert(limit) would be a cheap guard for future callers, but it's strictly optional — not a correctness bug in this PR.

  2. makeStack: deliberately keeps uniform gaps in PR 1. The MTStackRole comment says "so per-role metric choices can be added later," and makeStack: still uses stretchStackGapAboveMin / BelowMin uniformly (the per-kind gap selection is item 5 in PR 2). This is the intended slice boundary — just confirming the gap math is deliberately unchanged in PR 1, and the new role parameter currently only feeds the cramped/style derivation. No action needed.

Both points are non-blocking. The refactor is clean, well-tested, and the behavior-preserving claim holds.

@kostub kostub merged commit 8813444 into master Jun 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant