\bmod as a binary operator - #264
Conversation
Adds \bmod to the operators block of +supportedLatexSymbols as a plain Bin atom with ASCII nucleus "mod". It renders upright because changeFont's italic remap only applies to Variable/Number atoms, and serialization round-trips automatically via the auto-populated reverse (nucleus, type) map.
Characterization tests over existing finalize/typesetter machinery: a Bin \bmod with no left/right operand demotes to Unary (TeX Rule 5), the resulting display never trips the (Open, Bin) kMTSpaceInvalid assert, the rendered glyphs are the upright ASCII "mod" rather than italic mathematical alphanumerics, and a demoted Bin still serializes back to \bmod via the Un-to-Bin fallback in latexSymbolNameForAtom:. No production change required.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughChangesAdds bmod LaTeX support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
iosMathTests/MTModularArithmeticTest.m (1)
118-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actual binary-operator spacing.
XCTAssertNoThrowonly proves that layout construction avoids an invalid-spacing assertion; it does not verify that\bmodreceives binary-operator spacing. Add a layout/spacing assertion for an interior expression such as17 \bmod 5, ideally compared with anotherBinatom such as+.🤖 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/MTModularArithmeticTest.m` around lines 118 - 124, Extend testBmodAtBoundariesBuildsDisplay to inspect layout spacing for an interior expression such as “17 \bmod 5”, asserting that \bmod uses the same binary-operator spacing as a comparable “17 + 5” expression. Keep the existing boundary no-throw assertions unchanged.
🤖 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.
Nitpick comments:
In `@iosMathTests/MTModularArithmeticTest.m`:
- Around line 118-124: Extend testBmodAtBoundariesBuildsDisplay to inspect
layout spacing for an interior expression such as “17 \bmod 5”, asserting that
\bmod uses the same binary-operator spacing as a comparable “17 + 5” expression.
Keep the existing boundary no-throw assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d82f327-d2de-4409-959f-2c628e10f78f
📒 Files selected for processing (2)
iosMath/lib/MTMathAtomFactory.miosMathTests/MTModularArithmeticTest.m
kostub
left a comment
kostub
left a comment
There was a problem hiding this comment.
Code review — PR 1 (\bmod as a binary operator)
Reviewed against docs/plans/2026-07-25-modular-arithmetic.md (PR 1, Tasks 1-2) and docs/lld/2026-07-13-modular-arithmetic.md. Verified locally on a detached worktree at c298171: swift test --filter MTModularArithmeticTest → 7/7 pass; full suite → 446 tests, 0 failures.
Strengths
- The production change is exactly one symbol-table entry, and it is the right modeling. TeX defines
\bmodas\mathbin{\operator@font mod}, sokMTMathAtomBinaryOperatorwith an ASCII nucleus is faithful — not an Op like\sin. - Multi-character nuclei already have precedent in the same
+supportedLatexSymbolstable (\log,\sin,\lim), so nothing new is being asked of the reverse map or the typesetter. - Boundary-demotion coverage is genuinely valuable, not box-ticking:
(Open, Bin)iskMTSpaceInvalidand asserts atMTTypesetter.m:1093, so a\bmodat list start is a real crash surface. Good that Task 2 pins it. - The Un→Bin fallback in
+latexSymbolNameForAtom:pre-dates this PR;testDemotedBmodSerializeslocking it in for the new symbol is the right call.
I also probed some edge cases the tests don't cover — \left(\bmod b\right), \{\bmod b\}, \frac{\bmod b}{c}, a^{b \bmod c} — all build cleanly without tripping the assert. No action needed; noting so it isn't re-derived later.
Issues
1. (Important) The new test file is not registered in iosMath.xcodeproj/project.pbxproj.
The project is a classic pbxproj — zero fileSystemSynchronizedGroups — so files must be added explicitly. Every other tracked test file has 4 references:
4 MTColorDecoderTest.m 4 MTMathListBuilderTest.m
4 MTFontManagerTest.m 4 MTMathListTest.m
4 MTInkWidthTest.m 4 MTTypesetterTest.m
0 MTConcurrencyTest.m 0 MTModularArithmeticTest.m <-- this PR
MTInkWidthTest.m is recent, so the convention is live; MTConcurrencyTest.m looks like a pre-existing oversight rather than a precedent. The consequence is that xcodebuild test -project iosMath.xcodeproj -scheme iosMath — the documented iOS test path in CLAUDE.md — silently skips all 7 tests, and will keep skipping the feature's tests through PR 2 and PR 3. The plan calls this "mechanical; not required for swift test", which is true but means it currently belongs to no PR.
MacOSMath.xcodeproj contains no Obj-C test files at all, so only the iOS project needs the change.
2. (Important) Nothing asserts the spacing — the one thing that makes \bmod a Bin instead of an Ord.
Tests cover type, nucleus, fontStyle, serialization, discoverability, uprightness, demotion, and no-crash. The rendered gap is untested. Measured locally at 20pt:
17 \bmod 5 width 76.669
17 \mathrm{mod} 5 width 67.780
delta 8.889 = 2 x 4mu (muUnit = 20/18 = 1.111)
That 4mu-per-side result is precisely the value LLD §4.3 knowingly trades against LaTeX's 5mu, and it is what the PR goal claims ("renders upright with binary-operator spacing"). A width comparison against \mathrm{mod}, or an assertion on the sub-display x offsets, would pin the deviation and catch any future change to the binary row of getInterElementSpaces().
3. (Minor-to-Important) renderedTextForDisplay: fails silently on unhandled display classes.
It returns @"" for anything that is neither MTCTLineDisplay nor MTMathListDisplay — MTFractionDisplay, MTRadicalDisplay, MTGlyphDisplay, MTLargeOpLimitsDisplay, MTAccentDisplay. Paired with the negative assertion in testBmodRendersUpright:
XCTAssertFalse([text containsString:@"\U0001D45A"], @"italic m in %@", text);a traversal that dropped the subtree would pass vacuously. This helper is stack infrastructure — PR 3's \mod{n^2}^3 cases put content inside script displays — so it is worth hardening now: XCTFail on an unhandled MTDisplay subclass, or at minimum XCTAssertGreaterThan(text.length, 0) before any negative assertion.
4. (Minor) Comment attributes \bmod to amsmath.
The comment opens with "LaTeX kernel's binary mod" and then says the spacing "stands in for amsmath's hand-tuned 5mu". \bmod is defined in latex.ltx; \pmod/\mod/\pod are the amsmath ones. Suggest "LaTeX kernel's 5mu".
5. (Minor) The script-style deviation is understated in the comment.
"(4mu text/display, 0 in scripts) stands in for amsmath's hand-tuned 5mu" reads as if the only delta were 1mu. In LaTeX the \mkern5mu on either side of mod is not \nonscript-guarded — only the \mskip-\medmuskip is — so LaTeX keeps 5mu in script style where iosMath drops to 0, i.e. a^{b \bmod c} renders with no gaps at all. LLD §4.3 item 3 acknowledges this; the code comment is where a future reader will actually look, so it should say so too.
6. (Minor) Line-number citations in comments will rot.
The new comment and several test comments cite MTTypesetter.m:539-545, MTMathList.m:1709-1716, MTMathAtomFactory.m:190-239, MTMathAtomFactory.m:205-212. I checked them: the first two are accurate today; 205-212 is off by roughly two lines (the Un→Bin fallback sits at 204-209); 190-239 spans two unrelated methods. Naming the symbol (+preprocessMathList:, +addLatexSymbol:value:, +latexSymbolNameForAtom:) survives edits. Note the nearby pre-existing comment on +latexSymbolNameForAtom: already follows that style.
7. (Minor) No CHANGELOG entry anywhere in the stack.
grep -n "CHANGELOG" docs/plans/2026-07-25-modular-arithmetic.md returns nothing across all three PRs, yet every feature in v2.3.0 through v2.5.0 has one. Landing it in PR 3 is reasonable, but right now it is in no PR's plan.
Assessment
No blocking correctness defects — the symbol entry is right, the tests are real, and the suite is green. Issue 1 should be fixed before merge. Issues 2 and 3 are worth doing in this PR rather than later, since the test helper becomes shared infrastructure for PR 2 and PR 3. The rest are cleanups.
This reverts commit 8d43971. PR #265 added MTMacroAtom, the kMTMathAtomMacro type, a two-phase -finalized (expandMacros, then the existing reclassifying pass), and -transferScriptsToExpansion:, so that \pmod{n} would serialize back to \pmod{n} rather than to its expansion. Nothing else uses any of it, and \pmod/\mod/\pod themselves had not landed yet — so on master today this is ~460 lines of model-layer machinery with no caller. Preserving the command name through serialization is not an acceptance criterion for the feature (PRD §10 lists eight; none concern serialization). It appears only as an open question for the LLD (§9.5), and PRD §11's own reference table already describes \pmod's atom head as "Space + Open + Ord …" — an atom sequence, i.e. the expansion. iosMath already declines the same fidelity elsewhere: \implies serializes as \Longrightarrow. \pmod, \mod and \pod land in #266 instead as a parse-time expansion, the way TeX defines them. That also turns out to be the more faithful of the two: with no macro atom to carry them, \pmod{n}^2 attaches ^2 to the ")" exactly as LaTeX does, rather than transferring it onto the last scriptable atom of the expansion. MTMathList.h and MTMathList.m return to their state at e278be3. The \bmod tests from #264 are unaffected. 446 tests, 0 failures. Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Part 1 of 3 in the modular-arithmetic stack.
Plan:
docs/plans/2026-07-25-modular-arithmetic.mdLLD:
docs/lld/2026-07-13-modular-arithmetic.mdGoal
\bmodparses, renders upright with binary-operator spacing, and round-trips through serialization. Independent of the macro mechanism — it is a single symbol-table entry, and it ships user-visible value on its own.Commits
[item 1] Add \bmod as a binary-operator symbol[item 2] Test \bmod boundary demotion and upright renderingStack
\bmodas a binary operatorMTMacroAtomand two-phasefinalized🤖 Generated with Claude Code
https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Summary by CodeRabbit
\bmodcommand.\bmodcan be parsed and serialized reliably, including cases where the operator appears at a boundary.