Macro registry, template parsing, and the three commands - #266
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 (6)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThe parser adds built-in ChangesBuilt-in modular arithmetic macros
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Parser as Parser<br/>(buildInternal)
participant MacroLookup as macroAtomForCommand
participant ArgParser as requiredArgumentWithError
participant PrefixParser as buildFromString (fresh instance)
participant MTMacroAtom
Parser->>MacroLookup: resolve pmod, mod, or pod
MacroLookup->>ArgParser: parse required argument
ArgParser-->>MacroLookup: argument parsed
MacroLookup->>PrefixParser: parse prefix string
PrefixParser-->>MacroLookup: prefix nodes
MacroLookup->>PrefixParser: parse suffix string
PrefixParser-->>MacroLookup: suffix nodes
MacroLookup->>MTMacroAtom: construct with command, arguments, prefix, suffix
MTMacroAtom-->>Parser: macro atom ready
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kostub
left a comment
kostub
left a comment
There was a problem hiding this comment.
Review — PR 3 (diff vs feature/modular-arithmetic-pr2 only)
Verified locally: worktree at b8fdc60, swift test → 520 tests, 0 failures, clean tree.
Overall this is a tight, well-scoped landing. The registry is genuinely parser-owned and file-private, buildTemplate: really does use a fresh builder (no state swap/restore), macroAtomForCommand: falls through without setting an error for non-macros, and the two-phase finalized seam from PR 2 is not disturbed. Test coverage is unusually good for a parser change — the "macro vs. hand-written expansion" geometry test in particular is the right acceptance proof that render/ is untouched.
Things I checked and can confirm are correct
amsmath fidelity — the gap values are exactly right. Checked against TeX Live 2025 amsmath.sty (lines 907-912):
\DeclareRobustCommand{\pod}[1]{\allowbreak
\if@display\mkern18mu\else\mkern8mu\fi(#1)}
\DeclareRobustCommand{\pmod}[1]{\pod{{\operator@font mod}\mkern6mu#1}}
\DeclareRobustCommand{\mod}[1]{\allowbreak\if@display\mkern18mu
\else\mkern12mu\fi{\operator@font mod}\,\,#1}\pod→ inline\mkern8mu(#1)— matches the template exactly.\pmod→ inlining\podgives\mkern8mu({\operator@font mod}\mkern6mu#1)— matches, modulo the{…}group (which is an Ord noad in TeX and therefore spacing-neutral here; flattening it is fine, and it is why the template's#1is at top level — see the nested-#Nnote below).\mod→\mkern12mu{\operator@font mod}\,\,#1.\,is\mskip\thinmuskip= 3mu, so\,\,= 6mu, matching the template's\mkern6mu. (The kern-vs-glue distinction is only observable under line-breaking/stretch, which iosMath doesn't do.)\operator@font≡ upright roman ≡\mathrm. ✔- Dropped
\allowbreak/\penalty900are line-break hints — no-ops here. \if@display18mu is a documented non-goal (PRD §3.1 / LLD §4.2).
Script-style behavior is correct and matches TeX. Neither \pmod/\mod/\pod uses \nonscript, so in real TeX the mu kerns survive into script styles but shrink, because mu is 1/18 of the current style's quad. iosMath matches: MTTypesetter.m:634 uses _styleFont.mathTable.muUnit and MTFontMathTable.m:71 is _fontSize/18 on the style font. So x^{\pmod{n}} gets a proportionally smaller gap, as amsmath does. (Nit: testLeadingGapWidths only exercises display style — a one-line script-style case would lock this in.)
Deviation 1 (\pmod{\frac} is not a parse error) — sound. Confirmed the mechanism directly. requiredArgumentWithError: ends in [self buildInternal:YES], the same one-token reader \frac uses for its own operands, so \frac's numerator/denominator reads see the } immediately and each come back empty, not missing — identical to bare top-level \frac at EOF. The substituted \pmod{{n} row does exercise the original intent: the MTParseErrorMismatchBraces comes from the inner recursion hitting EOF with an unclosed group (MTMathListBuilder.m:548), not from -build's trailing leftover-characters check, so it is genuine inner-error propagation per LLD §6. testRequiredArgumentPropagatesInnerError ({\notacommand} → MTParseErrorInvalidCommand) covers the same invariant from a second angle. Good call keeping the observed behavior and characterizing it rather than forcing the plan's literal string.
Deviation 2 (\mkern8.0mu(n)) — sound. 8/12/6 mu have no named commands in +spaceToCommands (which covers 3/4/5/-3/18/36mu), so the \mkern%.1fmu fallback is the only correct output, and it re-parses.
Deviation 3 (test-block placement) — fine, purely mechanical.
+supportedMacroNames vs. file-private MTMacroDefinition — coherent. The method's return type leaks no private type, and returning .allKeys matches the existing convention of +[MTMathAtomFactory supportedLatexSymbolNames] (also .allKeys, also unordered), so there's no new API-shape inconsistency. testSupportedMacroNames even asserts the two surfaces stay disjoint, which is the right guard given macroAtomForCommand: runs before atomForCommand: and would silently shadow a symbol of the same name.
Findings
Important — requiredArgumentWithError: is not fail-loud against stop commands, and silently swallows a \\ row break
MTMathListBuilder.m:212-227 blacklists }, ^, _, & as "argument position is empty", but a following stop command (\\, \cr, \right, \end) is not in the blacklist, so control reaches [self buildInternal:YES], which returns an empty list with no error — the exact silent-permissiveness the method's own header comment says it exists to close.
Measured on this branch (all parse successfully, no error):
| input | raw serialization |
|---|---|
\left(\pmod\right) |
\left( \pmod{}\right) — argument silently empty |
x \pmod \\ y |
x\pmod{\\ y} — the \\ builds a table nested inside \pmod's parens |
\begin{matrix}a\pmod\\b\end{matrix} |
\begin{matrix}a\pmod{}b\end{matrix} — the row break is consumed and the matrix loses a row |
The last one is the one that bothers me: a \pmod \\ b inside a matrix silently renders as a single row. That's wrong output rather than an error, which is the failure mode this repo explicitly tries to avoid, and it's new behavior — before this PR \pmod was simply an unknown command and errored.
Yes, \sqrt has the identical hole (x\sqrt{\\ y} — I checked), and migrating \sqrt is correctly out of scope. But requiredArgumentWithError: is new code whose whole stated purpose is to be stricter than buildInternal:YES, and it's cheap to close: after the }^_& check, peek for \ and, if readCommand would yield one of the stop commands, raise MTParseErrorMissingArgument (restoring the read position first). \over/\atop/\choose already error correctly via stopCommand:'s oneChar guard, and \right/\end outside their opener already error for unrelated reasons — so in practice this is about \\ and \cr, plus \right/\end when the opener is present.
If you'd rather not widen the guard in this PR, please at least add the three rows above as characterization tests so the behavior is recorded rather than accidental.
Minor — nested #N is not reachable today, but this PR moves it one registry edit away
Answering the question raised in the PR body: no, this PR does not make the nested-#N follow-up reachable in practice. The registry is a file-private compile-time constant and none of the three templates put #1 inside braces, so no user input can trigger it. I confirmed the failure mode empirically with a hand-built template \mkern8mu({\mathrm{mod}\mkern6mu#1}): debug asserts loudly ("Macro parameter placeholder #1 escaped a template" from finalizedAssumingNoMacros), Release renders a literal #1.
What's worth flagging is that the natural transcription of amsmath's own \pmod — \pod{{\operator@font mod}\mkern6mu#1} — nests exactly this way. The template here is correct only because it was deliberately flattened, and nothing in the code records that constraint. A cheap guard in buildTemplate: (or in +builtinMacros) that walks the parsed template and rejects an MTMacroParameterAtom below top level would make the invariant self-enforcing rather than depending on whoever edits the dictionary next.
Minor — the registry's declared argumentCount is never validated against its template
argumentCount is declared per entry (MTMathListBuilder.m:1788) and independently the template contains #N; nothing checks they agree. Confirmed the consequence: with argumentCount:1 and template (#1,#2), -[MTMacroAtom expansion] asserts in debug but takes the continue branch in Release — the #2 is silently dropped from the output. Same one-line fix location as the note above: while walking the parsed template, assert max(#N) <= argumentCount. Both checks together turn two latent silent-wrong-output paths into a startup-time programming error, which is what they are.
(Related: +builtinMacros's templates are re-parsed from scratch on every macro invocation — a fresh MTMathListBuilder, a malloc, and a full parse per \pmod in the input. MTMacroAtom deep-copies the template anyway, so caching the parsed golden list on MTMacroDefinition would be safe and would be the natural place to hang the validation above. Not a correctness issue; 8 atoms is cheap.)
Minor — no CHANGELOG.md entry
This is the PR that makes the feature user-visible, and CHANGELOG.md follows a per-feature-per-release convention (v2.5.0 lists array, \cancel, etc.). Nothing across the whole 3-PR stack touches it, so \bmod, \pmod, \mod, \pod would ship undocumented. Worth a line here covering all three PRs.
Nit — ## in a template
## is rejected (MTParseErrorInvalidCharacter) where TeX would read it as an escaped literal #. Irrelevant for a fixed internal registry, and arguably the safer default; noting only for completeness. #12 correctly reads as #1 followed by 2, matching TeX's one-digit rule. #, #0, #x are all correctly rejected and covered by testBuildTemplateRejectsMalformedPlaceholder.
Nit — buildTemplate: recursion is not depth-limited across builder instances
Each buildTemplate: call gets a fresh builder, so _recursionDepth resets to 0. A self-referential or mutually-recursive template would stack-overflow rather than hitting MTParseErrorNestingTooDeep. Not reachable with a hardcoded, non-recursive registry; would matter if LLD §8.2's user-defined registry ever lands.
Per the review request I've deliberately not re-reported the three known PR #265 follow-ups (unhandled kMTMathAtomMacro in MTTypesetter.m's switch, top-level-only #N, missing iosMath.xcodeproj entries), and I've treated the plan as authoritative over LLD §6/§7.3 for \mod{\;}^2.
Not approving — the stop-command hole in requiredArgumentWithError: is worth a decision before merge (fix, or characterize with tests).
34dd3f7 to
36d72d2
Compare
Adds a fail-loud required-argument reader for future macro invocations. -buildInternal:YES alone is silently permissive at EOF and before a following }/^/_/&; \sqrt and friends keep that behavior (out of scope here), but macros must error instead.
Adds an opt-in _templateMode ivar and a +buildTemplate: entry point that parses a fresh builder instance with placeholder recognition enabled. '#' in ordinary user input is untouched and still raises MTParseErrorInvalidCharacter.
Parser-owned, file-private registry mapping each of the three macro names to its argument count and amsmath-exact LaTeX template string. +supportedMacroNames exposes discovery without overlapping the symbol table's +supportedLatexSymbolNames (\bmod stays symbol-only).
Add -macroAtomForCommand: which looks up the built-in macro registry, reads arguments with the existing -requiredArgumentWithError: reader, and parses the golden template with a fresh builder instance via +buildTemplate:. Wire it into -buildInternal:oneCharOnly:stopChar: ahead of -atomForCommand: so a macro command yields a single MTMacroAtom that flows through the existing shared script-attach + append + oneCharOnly tail with no new logic. Non-macro commands are unaffected: macroAtomForCommand: returns nil without setting an error when the command isn't in the registry, so dispatch falls through to atomForCommand: exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Test-only additions covering \pmod, \mod, and \pod parsed from LaTeX
end-to-end: parsing, macro expansion, and superscript/subscript
attachment behavior.
Notably documents that the `^2` in `\mod{\;}^2` lands on the `d` of
"mod" rather than the trailing space/argument — a deliberate,
documented divergence from LLD §6/§7.3, per the plan's preamble.
Adds \pmod/\mod/\pod parse-error rows to getTestDataParseErrors() and
serialization round-trip + raw-vs-finalized tests to
MTModularArithmeticTest.m, per plan Task 15.
Two deviations from the plan's literal expectations, verified against
actual behavior rather than reverted:
- `\pmod{\frac}` is NOT a parse error: requiredArgumentWithError: reads
arguments with the same one-token buildInternal:YES reader that
\frac itself uses for its numerator/denominator, so \frac with no
operands degrades to \frac{}{} (same as bare top-level \frac at
EOF) instead of failing. Swapped in \pmod{{n} (an actually
unbalanced brace), which does propagate MTParseErrorMismatchBraces,
and added testFracWithNoArgumentsIsNotAnErrorInsideMacroArgument to
characterize the \frac case directly.
- \pod{n}.finalized serializes to "\mkern8.0mu(n)", not "(n)": the
leading 8mu kern has no named LaTeX command, so it serializes via
the documented \mkern%.1fmu fallback rather than disappearing.
Full suite: 516 tests passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
…sion Adds the rendering acceptance tests from plan Task 16: upright (non-italic) "mod" text, byte-identical layout geometry (width/ascent/descent/subdisplay positions) between a macro and its hand-written-out expansion, exact 8mu/12mu leading-gap widths, and a no-throw smoke test across representative macro inputs. Imports MTFontMathTable.h for muUnit. No production changes: iosMath/render/ is untouched by this feature (LLD §1), and all four tests pass immediately, which is the acceptance proof that the renderer really is unchanged and the macro expansion really does lay out identically to the equivalent hand-typed LaTeX. Full suite: 520 tests passing. This is the final item of the final PR in the modular-arithmetic plan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
b8fdc60 to
9b21b2d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
iosMath/lib/MTMathListBuilder.m (1)
1805-1808: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
supportedMacroNamesorder is non-deterministic.
allKeyson anNSDictionaryhas unspecified ordering. If any caller or test asserts an exact ordered array (e.g.@[@"pmod", @"mod", @"pod"]), this can flake across runs/OS versions even though the underlying registry never changed.♻️ Proposed fix: return a deterministic order
+ (NSArray<NSString *> *) supportedMacroNames { - return [MTMathListBuilder builtinMacros].allKeys; + return [[MTMathListBuilder builtinMacros].allKeys sortedArrayUsingSelector:`@selector`(compare:)]; }🤖 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 `@iosMath/lib/MTMathListBuilder.m` around lines 1805 - 1808, Update supportedMacroNames to return macro names in a deterministic order instead of directly exposing builtinMacros.allKeys. Sort the returned names using the project’s standard string ordering while preserving the existing macro registry contents.
🤖 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 `@iosMath/lib/MTMathListBuilder.m`:
- Around line 1805-1808: Update supportedMacroNames to return macro names in a
deterministic order instead of directly exposing builtinMacros.allKeys. Sort the
returned names using the project’s standard string ordering while preserving the
existing macro registry contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2c7e41-6517-466f-9de2-7b7c8e054759
📒 Files selected for processing (4)
iosMath/lib/MTMathListBuilder.hiosMath/lib/MTMathListBuilder.miosMathTests/MTMathListBuilderTest.miosMathTests/MTModularArithmeticTest.m
Fail loud on a stop command in a macro argument slot.
-requiredArgumentWithError: blacklisted }/^/_/& but not a following stop
command, so control reached -buildInternal:YES, which handed it to
-stopCommand:. For \\ and \cr that ended the row and returned it as the
"argument" — `\begin{matrix}a\pmod\\b\end{matrix}` silently lost a row —
and for \right/\end it returned an empty list with NO error. Wrong output
rather than a diagnostic, which is what this method exists to prevent.
Guard the whole set -stopCommand: recognizes (\right, \over, \atop,
\choose, \brack, \brace, \\, \cr, \end) rather than only the two that
leaked: none can begin an argument, so one uniform rule beats four special
cases. This changes `x\pmod\over y` from MTParseErrorInvalidCommand (via
-stopCommand:'s oneChar guard) to MTParseErrorMissingArgument, which names
the actual problem. Only the argument POSITION is guarded — a stop command
after a complete argument keeps its normal meaning.
Adds -peekCommand (read a command, restore the position) and a
+stopCommands set beside -stopCommand: so the two stay coupled.
Validate a macro template against its declared arity.
buildTemplate: gains an argumentCount: parameter and checks the three
invariants -[MTMacroAtom expansion] relies on but cannot enforce:
- No #N above the declared argumentCount. Release carried the placeholder
through to render as a literal "#2".
- No #N below top level. -expansion substitutes only top-level
placeholders. amsmath's own \pmod nests exactly this way
(\pod{{\operator@font mod}\mkern6mu#1}), so the flattened built-in
template was correct only by deliberate choice and nothing recorded it.
- No script on a top-level #N. -expansion replaces the placeholder with the
argument list wholesale, dropping any script the placeholder carried.
Found while writing the check above; same silent-wrong-output family.
All three are programming errors in +builtinMacros — no LaTeX input can
reach them — so they NSAssert and reject. Rejecting also closes the Release
path, where the assert is compiled out but the template is still refused.
MTContainsMacroParameter (MTMathList.m) is un-staticked and declared in
MTMacroParameterAtom.h to do the nested walk; it already visits every
container an atom can hold.
The parsed template is deliberately NOT cached on MTMacroDefinition:
parsing one reaches -macroAtomForCommand:, which calls +builtinMacros, so
caching inside its dispatch_once would re-enter it on the same thread and
deadlock. Reasoning recorded at the registry.
Also: sort +supportedMacroNames (NSDictionary key order is unspecified, so
an ordered assertion by a caller could flake); add a script-style case to
the leading-gap test, locking in that the mu kerns shrink with the style as
amsmath's do; CHANGELOG entry covering all three PRs in the stack.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Line 3: Update the v2.6.0 changelog heading to “Unreleased” instead of the
future date, and retain the existing version and entries until the release is
officially published.
In `@iosMath/lib/internal/MTMacroParameterAtom.h`:
- Around line 33-39: Add MTMacroParameterAtom.h and its implementation file to
the Xcode project and the relevant target membership, ensuring the MTMathList.m
reference to MTContainsMacroParameter links successfully.
In `@iosMath/lib/MTMathListBuilder.m`:
- Around line 1217-1219: Update MTTypesetter to explicitly handle MTMacroAtom
before returning or rendering parsed macros, ensuring \pmod, \mod, and \pod are
expanded or otherwise processed into layoutable content. Preserve existing
handling for non-macro atoms and use the established macro expansion path where
available.
🪄 Autofix (Beta)
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: 32e4db45-3e10-4f5e-b34c-76dde0d5c5c0
📒 Files selected for processing (5)
CHANGELOG.mdiosMath/lib/MTMathList.miosMath/lib/MTMathListBuilder.miosMath/lib/internal/MTMacroParameterAtom.hiosMathTests/MTModularArithmeticTest.m
| @@ -1,5 +1,8 @@ | |||
| ## Changelog | |||
|
|
|||
| ### v2.6.0 (2026-07-27) | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not publish a future-dated release entry.
v2.6.0 is dated July 27, 2026, but today is July 26, 2026. Mark this section “Unreleased” until publication, or update the date when the release is cut.
🤖 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 `@CHANGELOG.md` at line 3, Update the v2.6.0 changelog heading to “Unreleased”
instead of the future date, and retain the existing version and entries until
the release is officially published.
| @@ -1,5 +1,8 @@ | |||
| ## Changelog | |||
|
|
|||
| ### v2.6.0 (2026-07-27) | |||
There was a problem hiding this comment.
Nothing is released. Why did CHANGELOG get updated? Please revert.
There was a problem hiding this comment.
this is weird and difficult to maintain. What is this function trying to do and why is it necessary. can we remove it?
| } | ||
|
|
||
| // The built-in macro registry. Each entry is amsmath's exact INLINE expansion | ||
| // (LLD §3.3): the 8mu/12mu leading and 6mu inner gaps are amsmath's literal \mkern |
Two review rounds accreted guards the LLD does not specify, and they duplicate
checks that already existed. Diffing the branch against docs/lld/2026-07-13:
- kMTMaxMacroExpansionDepth / -expansionAtDepth: / RunawayMacroExpansion.
LLD 8.2 scopes a depth guard as roadmap-only: "the three built-ins are
non-recursive and terminate by construction, so nothing here blocks this PR."
It also fired on legal input — the parser accepts nesting up to
kMTMaxRecursionDepth (150), so a 33-deep chain parsed fine and then threw an
uncaught exception on MTMathUILabel's render path. Removed; expansion
recursion is already bounded by the nesting -buildInternal: accepted.
- +validateTemplate:argumentCount:. Not in the LLD, and two of its three checks
were already covered: the nested-#N check duplicates the assert in
-[MTMacroAtom initWithCommand:arguments:templateExpression:], and the arity
check duplicates the one in -expansion. Removed, and buildTemplate: goes back
to the LLD's single-argument form.
Its third check (a script on a top-level placeholder) was genuinely new, so it
moves into the initializer loop that already walks the template, beside the
nested-placeholder assert it belongs with — two lines rather than a new method
in the builder.
- MTContainsMacroParameter goes back to static; only the deleted duplicate
needed it exported from MTMacroParameterAtom.h.
The stop-command guard in -requiredArgumentWithError: stays. That one is a real
gap the LLD missed — 3.3 lists only "} ^ _ &", and without it
\begin{matrix}a\pmod\\b\end{matrix} silently lost a row.
The runaway-depth test is replaced by its inverse: deep-but-legal nesting (8,
32, 33, 64 programmatic; 40 through the parser) must expand without throwing.
No user-facing LaTeX behavior changes. 535 tests pass; release build clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Overrules LLD §4.1, which rejected code-built expansions because "\newcommand
would need this template engine built from scratch anyway." That premise does not
survive the implementation: substitution only reaches the top level of a template,
so \newcommand{\abs}[1]{\left|#1\right|} — the example §8.2 cites as the payoff —
is rejected by MTMacroAtom's own nested-placeholder assert. MTMathList.h said so
outright ("a future user-facing \newcommand would need substitution to descend
into sub-lists first"). The engine was paying for a reuse it cannot deliver, and
\newcommand appears nowhere in the PRD.
MTMacroDefinition now holds a prefix and a suffix LaTeX string; \pmod{n} expands
to prefix + arguments + suffix:
pmod \mkern8mu(\mathrm{mod}\mkern6mu … )
mod \mkern12mu\mathrm{mod}\mkern6mu … (none)
pod \mkern8mu( … )
Both halves are ordinary LaTeX parsed by the ordinary builder, so this deletes
template mode, #N parsing, +buildTemplate:, MTMacroParameterAtom (60 lines), the
recursive MTContainsMacroParameter walker (51 lines), and both template-invariant
asserts — the placeholder cannot be misplaced when there is no placeholder.
MTMacroAtom keeps command + arguments, which is the requirement that justified it
existing: \pmod{n} still serializes back as \pmod{n}, and ^/_ still attach to one
atom via the builder's shared tail.
Cost: an expansion that interleaves fixed text between several arguments is no
longer expressible. None of the three built-ins needs that, and \newcommand would
need the descending substitution above regardless.
No user-facing LaTeX behavior changes — parsing, expansion, spacing, serialization
and round-trip tests are unchanged and still pass. 527 tests, 0 failures; release
build clean. LLD §4 annotated with the supersession.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
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>
|
Superseded by #268, which rebuilds this and #265 as a single PR off the reverted master. Same architecture (MTMacroAtom expanded in -finalized), without the |
1 participant
Part 3 of 3 in the modular-arithmetic stack. This is the PR that makes the feature user-visible.
Plan:
docs/plans/2026-07-25-modular-arithmetic.mdLLD:
docs/lld/2026-07-13-modular-arithmetic.mdGoal
\pmod{n},\mod{n},\pod{n}parse from LaTeX intoMTMacroAtoms, with amsmath's exact inline gaps, upright "mod", fail-loud missing-argument errors, and round-tripping serialization.Commits
[item 10] Add MTParseErrorMissingArgument and requiredArgumentWithError:[item 11] Parse #N placeholders in macro-template mode[item 12] Add MTMacroDefinition registry for pmod/mod/pod[item 13] Dispatch \pmod, \mod, \pod to MTMacroAtom[item 14] Test end-to-end equivalence and script behavior through LaTeX[item 15] Test macro serialization round-trip and parse errors[item 16] Test macro rendering geometry against the written-out expansionFull suite green at 520 tests.
Deviations from the plan (all documented in commit messages)
\pmod{\frac}is not a parse error, contrary to the plan's literal parse-error table row.-requiredArgumentWithError:reads arguments with the same one-tokenbuildInternal:YESreader\fracitself uses, so a\fracwith no operands degrades to\frac{}{}exactly as a bare top-level\fracat EOF does, rather than failing. Verified directly and covered by a newtestFracWithNoArgumentsIsNotAnErrorInsideMacroArgument. The table row was swapped to\pmod{{n}— a genuinely unbalanced brace, which does propagateMTParseErrorMismatchBraces— preserving the row's real intent per LLD §6 (an inner parse error must propagate).\pod{n}.finalizedserializes as\mkern8.0mu(n), not the plan's literal(n). The leading 8mu kern has no named LaTeX command in+spaceToCommands, so it correctly serializes through the\mkern%.1fmufallback. Same root cause as the item-7 correction in PR 2.@implementation/@endpair or nest an@interfaceinside@implementation, neither of which compiles. Test-only category declarations were placed at file scope and test methods inside the single existing@implementation MTModularArithmeticTest.Known follow-ups raised in review of PR #265 (not addressed here)
kMTMathAtomMacrois unhandled inMTTypesetter.m'screateDisplayAtoms:switch, which has nodefault:— so-Wswitchis its exhaustiveness guard. A macro atom escaping expansion would be skipped and drivegetInterElementSpaceArrayIndexForTypetoreturn -1, an OOB index in Release (NS_BLOCK_ASSERTIONS=1).-[MTMacroAtom expansion]substitutes only top-level#N; a#1nested inside anMTMathGroupsurvives unsubstituted. Now reachable in principle, since this PR makes templates LaTeX strings where{#1}is natural — though none of the three built-in templates nest.MTMacroParameterAtom.h,MTModularArithmeticTest.m) are not iniosMath.xcodeproj, so the documentedxcodebuild testiOS path skips this feature's tests. Originates in PR 1.Stack
\bmodas a binary operator (\bmod as a binary operator #264)MTMacroAtomand two-phasefinalized(MTMacroAtom and two-phase finalized #265)🤖 Generated with Claude Code
https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Summary by CodeRabbit
New Features
\bmod,\pmod,\mod, and\pod.Bug Fixes
Tests