Spacing commands — kern, hspace, skip (item 4)#243
Conversation
…MathSpace Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces support for TeX spacing commands (such as \kern, \hspace, \hskip, \mkern, \mskip, and \mspace) by implementing a dimension parser and mapping commands to their allowed units (em or mu). It also adds comprehensive unit tests to verify parsing errors, spacing dimensions, and typesetter advances. The review feedback suggests defensive programming improvements in MTMathListBuilder.m, specifically guarding against a potential nil key when looking up spacing commands in the dictionary and checking for a NULL pointer before dereferencing outMu in readDimensionIntoMu:allowEm:command:.
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.
| } else { | ||
| // Spacing commands: \kern, \hspace[*], \hskip, \mkern, \mskip, \mspace. | ||
| // The table maps each command name to @YES (em or mu allowed) or @NO (mu only). | ||
| NSNumber* allowEm = [MTMathListBuilder spacingCommands][command]; |
There was a problem hiding this comment.
Always guard against nil keys before accessing an NSDictionary in Objective-C to prevent NSInvalidArgumentException crashes. If command is nil, the lookup [MTMathListBuilder spacingCommands][command] will crash. Although command is expected to be non-nil in the current parser flow, adding a defensive check ensures safety against future refactorings or unexpected inputs.
NSNumber* allowEm = command ? [MTMathListBuilder spacingCommands][command] : nil;References
- Always guard against
nilkeys before accessing or modifying anNSMutableDictionary(orNSDictionary) in Objective-C to preventNSInvalidArgumentExceptioncrashes.
| } | ||
| } | ||
|
|
||
| *outMu = sign * num.doubleValue * factor; |
There was a problem hiding this comment.
It is a good defensive programming practice to check if the pointer outMu is not NULL before dereferencing it. Dereferencing a NULL pointer will cause a crash. Although the current call site always passes a valid address of a local variable, adding this check prevents potential crashes if the method is reused or refactored in the future.
if (outMu) {
*outMu = sign * num.doubleValue * factor;
}
kostub
left a comment
There was a problem hiding this comment.
Code review — Spacing commands (item 4)
Reviewed against the plan (docs/plans/2026-06-28-box-spacing-layer.md, PR 2 / Task 4) and LLD. Built with swift build and ran the new tests plus targeted edge-case probes off ffcfb3a. All green; the implementation is faithful to the plan and the scanner behaves correctly. Not approving — leaving the call to the maintainer. No blocking issues found.
Strengths
- Faithful to the design: reuses the existing
MTMathSpaceatom (mu units), no new atom/display/export,em→mufactor 18 matches the existingquad/qquadtable inMTMathAtomFactory.m. - Sensible deviation from the plan's
@"hspace*"table key: keys onhspaceand consumes the trailing*in the dispatch, which is correct givenreadStringstops at the non-alphabetic*. The inline comment explains it. - Upgraded the
+spacingCommandstable todispatch_once(the plan sketch used a non-thread-safeif (!commands)); consistent with the file's other lazy tables. - Malformed/unsupported input fails loud with
MTParseErrorInvalidCommand(matches the LLD's error-code decision and the\cfrac[zzz]precedent). Verified\kern1pt,\kern1xx,\kern1eall set the error and produce no atom. - Round-trips:
\kern1em(space 18) re-serializes viaappendLaTeXToString:as\quad/\mkernX.Xmu, both re-parseable through this new path.
Minor (non-blocking)
1. Test gap — the "valid number, bad unit" error branch is uncovered.
All five new error cases hit either the no-digit branch (\kernabc, \hspace{abc}, \hspace{}) or the em-disallowed branch (\mkern{1em}). The else branch that emits "expects em or mu units, got: %@" (e.g. \kern1pt, \kern1xx) has no test, though I confirmed it works. Suggest adding to getTestDataParseErrors():
@[@"\\kern1pt", @(MTParseErrorInvalidCommand)], // valid number, unsupported unit2. \hspace *{1em} (whitespace between command and *) is rejected.
The * is only consumed when it is the immediate next character; with intervening space the scanner then sees * and errors. Real TeX tolerates the space. Acceptable given the documented em/mu-only scope, but worth a one-line note or a follow-up if broader TeX compatibility is desired.
3. Deviation from TeX semantics (intentional, per design).
\kern/\hspace/\hskip accept mu here, which real TeX does not (mu is only valid in math glue / \mkern/\mskip); and only em/mu are supported (no pt/cm/ex/…). This matches the LLD ("kern accepts em or mu") and the fail-loud principle, so not a bug — flagging only so the deviation is a conscious record.
Verification
swift build: clean.- New tests
testParseSpacingDimensions,testParseSpacingAliases,testParseGlueTailIgnored,testSpacingAdvances, and the 5 added error cases: all pass. - Edge probes confirmed:
\kern1.em→1 atom,\kern1emx→space + ordinaryx(glue-tail behavior), invalid units→error.
# Conflicts: # iosMath/lib/MTMathListBuilder.m # iosMathTests/MTMathListBuilderTest.m # iosMathTests/MTTypesetterTest.m
Owner review on PR #243 raised two minor items: 1. Test gap: the "valid number, unsupported unit" error branch in readDimensionIntoMu:allowEm:command: (the else that emits "expects em or mu units, got: %@") had no coverage. Add \kern1pt and \kern1xx to getTestDataParseErrors(). 2. \hspace *{1em} (whitespace between the command and the '*') was rejected where real TeX tolerates it. Skip spaces before consuming the optional '*'; a skipped run is harmless when no '*' follows since the dimension scanner also skips leading whitespace. Locked in with a new alias test. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Goal
Recognize
\kern,\mkern,\hspace,\hspace*,\hskip,\mskip,\mspacevia a newem/mulength scanner that emits the existingMTMathSpaceatom. No new atom, no new display — touches onlyMTMathListBuilder.m(plus tests).This is PR 2 of the box-and-spacing layer. It is independent of PR 1 and based directly on
master.Commits
[item 4]Add em/mu dimension scanner and spacing commands emitting MTMathSpaceImplementation
+spacingCommands— dispatch table:kern/hspace/hskipaccept em or mu;mkern/mskip/mspaceare mu-only.-readDimensionIntoMu:allowEm:command:— scanner handling optional{…}, sign, decimal mantissa, surrounding whitespace, andem/muunits. Malformed/unsupported input setsMTParseErrorInvalidCommand.-atomForCommand:— consumes the optional*for\hspace*, calls the scanner, returns[[MTMathSpace alloc] initWithSpace:mu].Tests
MTMathListBuilderTest:testParseSpacingDimensions,testParseSpacingAliases,testParseGlueTailIgnored, + 5 error cases ingetTestDataParseErrors().MTTypesetterTest:testSpacingAdvances.swift buildclean.References
docs/plans/2026-06-28-box-spacing-layer.md(PR 2, Task 4)docs/lld/2026-06-28-box-spacing-layer.md🤖 Generated with Claude Code