Add modular-arithmetic macros: \pmod, \mod, \pod - #268
Conversation
\bmod shipped in #264 as a plain symbol-table entry. The three remaining commands are macros: each takes one argument and expands to amsmath's exact inline form -- \pmod{n} to \mkern8mu(\mathrm{mod}\mkern6mu n). Rather than expanding at parse time, an invocation parses to a single MTMacroAtom holding the command name, its parsed arguments, and the fixed prefix/suffix LaTeX that brackets them. -[MTMathList finalized] expands every macro atom before the reclassifying pass runs, so the macro never reaches the typesetter, and the Bin/Unary boundary rules see the flat atom stream the macro stands for. Keeping the atom means \pmod{n} serializes back to \pmod{n} rather than to its expansion. Scripts written on the invocation transfer to the last script-capable atom of the expansion, so \pmod{n}^2 puts the 2 on the closing paren. On collision, or when there is no target, both scripts go on a fresh empty Ordinary -- the same fallback the builder already uses for x^2^3. The expansion halves are plain LaTeX parsed by the ordinary builder; there is no template syntax and no placeholder atom. That expresses one substitution region, which is what these three macros need. LLD 3.1/3.3 specified a #N template engine instead; 4.1 records why it was dropped. Commands that require an argument now fail loud when given none: MTParseErrorMissingArgument covers end-of-input, a }/^/_/& in argument position, and a stop command (\right, \\, \cr, \end, ...) which would otherwise end the enclosing list and be returned as the argument. Only macros route through the new check; \sqrt keeps its existing permissive behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
📝 WalkthroughWalkthroughAdds deferred macro atoms and built-in support for ChangesModular arithmetic macros
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Input
participant MTMathListBuilder
participant MTMacroAtom
participant MTMathList
participant MTTypesetter
Input->>MTMathListBuilder: parse modular-arithmetic command
MTMathListBuilder->>MTMacroAtom: store command, arguments, and template
MTMathList->>MTMacroAtom: expand macro during finalization
MTMacroAtom-->>MTMathList: return finalized atom sequence
MTMathList->>MTTypesetter: typeset finalized sequence
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
iosMath/lib/MTMathList.m (1)
111-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe "recursion is bounded by the parser" invariant does not hold for programmatically built atoms.
argumentsexposes mutableMTMathLists, so[macro.arguments[0] addAtom:macro]after init creates a cycle and-expansionrecurses until the stack overflows (the tests already rely on post-init argument mutation, e.g.testMacroAtomSerializationTracksArgumentMutation). A small depth budget threaded through-expansion/-expandMacros(or an identity set of macros currently expanding) would make the invariant enforced rather than assumed.Also applies to: 1905-1921
🤖 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/MTMathList.m` around lines 111 - 116, Update MTMathList expansion around expansion and expandMacros to guard against cycles introduced through mutable macro arguments after initialization. Thread a bounded recursion/depth budget or track macros currently being expanded, and stop expansion safely when the limit or cycle is detected while preserving normal nested-macro expansion and argument mutation behavior.
🤖 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`:
- Around line 3-4: Update the v2.6.0 entry in the changelog to remain unreleased
rather than using the future date, and revise its feature description to remove
\bmod as a newly added item while retaining the newly introduced \pmod, \mod,
and \pod macro changes.
In `@iosMath/lib/MTMathListBuilder.h`:
- Around line 110-112: Update the MTParseErrorMissingArgument documentation to
state that it applies specifically to missing macro arguments, while preserving
the listed end-of-input and delimiter cases and avoiding claims about all
commands such as \sqrt.
- Around line 54-60: Revise the documentation for the supported one-argument
macro command list in MTMathListBuilder so it describes only the symbol and
built-in-macro registries. Remove any wording that implies the two lists contain
every command accepted by the parser, while retaining the distinction that
symbol names and macros are maintained separately.
---
Nitpick comments:
In `@iosMath/lib/MTMathList.m`:
- Around line 111-116: Update MTMathList expansion around expansion and
expandMacros to guard against cycles introduced through mutable macro arguments
after initialization. Thread a bounded recursion/depth budget or track macros
currently being expanded, and stop expansion safely when the limit or cycle is
detected while preserving normal nested-macro expansion and argument mutation
behavior.
🪄 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: e9a0fd71-b55b-464f-8181-61bbeb3d536b
📒 Files selected for processing (9)
CHANGELOG.mdiosMath.xcodeproj/project.pbxprojiosMath/lib/MTMathList.hiosMath/lib/MTMathList.miosMath/lib/MTMathListBuilder.hiosMath/lib/MTMathListBuilder.miosMath/render/internal/MTTypesetter.miosMathTests/MTMathListBuilderTest.miosMathTests/MTModularArithmeticTest.m
| ### v2.6.0 (2026-07-28) | ||
| * Add **modular-arithmetic notation**: `\bmod` as a binary operator, and the `\pmod`, `\mod`, and `\pod` macros with amsmath's exact inline gaps and upright "mod" (#264, #268). `a \equiv b \pmod{n}` now renders as it does in LaTeX. The macros expand through a new internal macro atom, so a macro invocation serializes back to the command the author wrote rather than to its expansion. amsmath's wider display-style gap (18mu instead of 8/12mu) is not reproduced: a macro expands at parse time, before the render style is known. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the release metadata and \bmod scope.
July 28, 2026 is future-dated relative to July 27, 2026; keep this section unreleased until the tag is cut. Also, \bmod is unchanged in this PR, so do not present it as newly added.
Proposed fix
-### v2.6.0 (2026-07-28)
-* Add **modular-arithmetic notation**: `\bmod` as a binary operator, and the `\pmod`, `\mod`, and `\pod` macros ...
+### Unreleased
+* Add the `\pmod`, `\mod`, and `\pod` modular-arithmetic macros ... Existing `\bmod` remains a binary operator.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### v2.6.0 (2026-07-28) | |
| * Add **modular-arithmetic notation**: `\bmod` as a binary operator, and the `\pmod`, `\mod`, and `\pod` macros with amsmath's exact inline gaps and upright "mod" (#264, #268). `a \equiv b \pmod{n}` now renders as it does in LaTeX. The macros expand through a new internal macro atom, so a macro invocation serializes back to the command the author wrote rather than to its expansion. amsmath's wider display-style gap (18mu instead of 8/12mu) is not reproduced: a macro expands at parse time, before the render style is known. | |
| ### Unreleased | |
| * Add the `\pmod`, `\mod`, and `\pod` modular-arithmetic macros with amsmath's exact inline gaps and upright "mod" (`#264`, `#268`). Existing `\bmod` remains a binary operator. `a \equiv b \pmod{n}` now renders as it does in LaTeX. The macros expand through a new internal macro atom, so a macro invocation serializes back to the command the author wrote rather than to its expansion. amsmath's wider display-style gap (18mu instead of 8/12mu) is not reproduced: a macro expands at parse time, before the render style is known. |
🤖 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` around lines 3 - 4, Update the v2.6.0 entry in the changelog to
remain unreleased rather than using the future date, and revise its feature
description to remove \bmod as a newly added item while retaining the newly
introduced \pmod, \mod, and \pod macro changes.
| /// A command that requires an argument was given none — end of input, or a | ||
| /// `}`/`^`/`_`/`&` where the argument should be. | ||
| MTParseErrorMissingArgument, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope this error to macro arguments.
\sqrt still accepts omitted arguments by design, so “a command that requires an argument” overstates what currently emits this error.
Proposed fix
- /// A command that requires an argument was given none — end of input, or a
+ /// A built-in macro requiring an argument was given none — end of input, or a📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// A command that requires an argument was given none — end of input, or a | |
| /// `}`/`^`/`_`/`&` where the argument should be. | |
| MTParseErrorMissingArgument, | |
| /// A built-in macro requiring an argument was given none — end of input, or a | |
| /// `}`/`^`/`_`/`&` where the argument should be. | |
| MTParseErrorMissingArgument, |
🤖 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.h` around lines 110 - 112, Update the
MTParseErrorMissingArgument documentation to state that it applies specifically
to missing macro arguments, while preserving the listed end-of-input and
delimiter cases and avoiding claims about all commands such as \sqrt.
| @@ -1,5 +1,8 @@ | |||
| ## Changelog | |||
|
|
|||
| ### v2.6.0 (2026-07-28) | |||
There was a problem hiding this comment.
Do not add anything tot he changelog. There is no release.
MTMacroAtom held an NSArray of arguments, but every macro takes exactly one and nothing needed more. The array was the only reason MTMacroDefinition existed -- its sole non-string field was argumentCount -- so fixing arity at one collapses the registry to a plain command -> @[prefix, suffix] dictionary and removes the class, the deep-copy-array helper, the zero-argument serialization branch, and four loops. Also drops +supportedMacroNames, which had no caller outside its own tests. Comment density on the new code was 25-36% against a 3-14% baseline in these files. Cut roughly in half by removing the archaeology: notes arguing against the template engine that was never in this PR, LLD/PRD section citations, and line-number references that go stale. No behavior change. 524 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
iosMath.xcodeproj/project.pbxproj (1)
642-647: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
$(SRCROOT)/iosMath/lib/internalto bothHEADER_SEARCH_PATHSarrays.
MTMathList.m,MTMathListBuilder.m, andMTModularArithmeticTest.muse bare imports forMTMacroParameterAtom.h. WithUSE_HEADERMAP = NO, the inherited Xcode search paths cannot resolve this header.Package.swiftalready includes the required path.🤖 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.xcodeproj/project.pbxproj` around lines 642 - 647, Add $(SRCROOT)/iosMath/lib/internal to both HEADER_SEARCH_PATHS arrays in the Xcode project, alongside the existing iosMath/lib and render paths, so bare MTMacroParameterAtom.h imports resolve when USE_HEADERMAP is disabled.
🤖 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 `@docs/lld/2026-07-13-modular-arithmetic.md`:
- Around line 157-176: Add the appropriate language identifier, such as text, to
the fenced data-flow block in the documentation while preserving its contents
unchanged.
In `@iosMath/lib/MTMathList.m`:
- Around line 193-198: Update the exception reason in the kMTMathAtomMacro case
to reference the valid MTMacroAtom initializer
-initWithCommand:arguments:templateExpression: instead of the nonexistent
selector, leaving the exception behavior unchanged.
- Around line 1889-1890: Add a kMTMaxMacroExpansionDepth guard to the recursive
expandMacros/expansion flow, tracking nested macro expansion depth and stopping
or failing safely once the bound is reached. Preserve normal expansion behavior
for inputs within the limit and ensure nested arguments cannot recurse
indefinitely.
---
Outside diff comments:
In `@iosMath.xcodeproj/project.pbxproj`:
- Around line 642-647: Add $(SRCROOT)/iosMath/lib/internal to both
HEADER_SEARCH_PATHS arrays in the Xcode project, alongside the existing
iosMath/lib and render paths, so bare MTMacroParameterAtom.h imports resolve
when USE_HEADERMAP is disabled.
🪄 Autofix
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: be56345b-656a-4319-9a3f-407314381c10
📒 Files selected for processing (9)
Package.swiftdocs/lld/2026-07-13-modular-arithmetic.mdiosMath.xcodeproj/project.pbxprojiosMath/lib/MTMathList.hiosMath/lib/MTMathList.miosMath/lib/MTMathListBuilder.hiosMath/lib/MTMathListBuilder.miosMath/lib/internal/MTMacroParameterAtom.hiosMathTests/MTModularArithmeticTest.m
💤 Files with no reviewable changes (1)
- iosMath/lib/MTMathListBuilder.h
| ``` | ||
| LaTeX "\pmod{n}" | ||
| │ parse — read 1 arg via existing reader; parse template string | ||
| │ "\mkern8mu(\mathrm{mod}\mkern6mu#1)" with a FRESH builder → golden raw list | ||
| ▼ | ||
| MTMacroAtom{ command:"pmod", | ||
| arguments:[ MTMathList("n") ], | ||
| template:[Space8, Open"(", Ord"mod"(rom), Space6, «#1», Close")"] } | ||
| │ │ | ||
| │ +mathListToString: │ MTMathList.finalized (two-phase, model layer) | ||
| ▼ ▼ | ||
| "\pmod{n}" phase 1: expand ALL macros → RAW flat list | ||
| (command-faithful) deep-copy template, splice arguments into «#N»: | ||
| [Space8, Open"(", Ord"mod"(rom), Space6, <n copy>, Close")"] | ||
| ▼ | ||
| phase 2: existing finalization, run ONCE → macro-free list | ||
| │ MTTypesetter (unchanged) | ||
| ▼ | ||
| glyphs | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced data-flow block.
The fence at Line [157] has no language tag. Use text or another accurate identifier so markdownlint MD040 passes.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/lld/2026-07-13-modular-arithmetic.md` around lines 157 - 176, Add the
appropriate language identifier, such as text, to the fenced data-flow block in
the documentation while preserving its contents unchanged.
Source: Linters/SAST tools
| case kMTMathAtomMacro: | ||
| // The default would mint a plain MTMathAtom carrying type 22 — an atom | ||
| // that claims to be a macro but cannot expand. | ||
| @throw [NSException exceptionWithName:@"InvalidMethod" | ||
| reason:@"A macro atom cannot be created by type. Use -[MTMacroAtom initWithCommand:argument:prefix:suffix:] instead." | ||
| userInfo:nil]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale selector in the exception message.
The message names -[MTMacroAtom initWithCommand:argument:prefix:suffix:]. That selector does not exist. The designated initializer is -initWithCommand:arguments:templateExpression:, as used at line 1816.
🐛 Proposed fix for the message text
`@throw` [NSException exceptionWithName:@"InvalidMethod"
- reason:@"A macro atom cannot be created by type. Use -[MTMacroAtom initWithCommand:argument:prefix:suffix:] instead."
+ reason:@"A macro atom cannot be created by type. Use -[MTMacroAtom initWithCommand:arguments:templateExpression:] instead."
userInfo:nil];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case kMTMathAtomMacro: | |
| // The default would mint a plain MTMathAtom carrying type 22 — an atom | |
| // that claims to be a macro but cannot expand. | |
| @throw [NSException exceptionWithName:@"InvalidMethod" | |
| reason:@"A macro atom cannot be created by type. Use -[MTMacroAtom initWithCommand:argument:prefix:suffix:] instead." | |
| userInfo:nil]; | |
| case kMTMathAtomMacro: | |
| // The default would mint a plain MTMathAtom carrying type 22 — an atom | |
| // that claims to be a macro but cannot expand. | |
| @throw [NSException exceptionWithName:@"InvalidMethod" | |
| reason:@"A macro atom cannot be created by type. Use -[MTMacroAtom initWithCommand:arguments:templateExpression:] instead." | |
| userInfo:nil]; |
🤖 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/MTMathList.m` around lines 193 - 198, Update the exception reason
in the kMTMathAtomMacro case to reference the valid MTMacroAtom initializer
-initWithCommand:arguments:templateExpression: instead of the nonexistent
selector, leaving the exception behavior unchanged.
| // An argument may itself contain a macro. Re-scan so the result is macro-free | ||
| // at its top level, and so script transfer targets a real atom. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect macro expansion recursion and any depth bound.
rg -n 'MaxMacroExpansionDepth|expandMacros|expansionAtDepth|- \(MTMathList \*\)expansion' iosMath/lib/MTMathList.m
sed -n '1755,1925p' iosMath/lib/MTMathList.mRepository: kostub/iosMath
Length of output: 6957
🏁 Script executed:
#!/bin/bash
# Inspect all macro-expansion declarations, call sites, and macro-definition paths
rg -n --glob '*.{h,m}' 'expandMacros|expansionAtDepth|MaxMacroExpansionDepth|MTMacroAtom|defineMacro|macro' iosMath
printf '\n--- relevant declarations ---\n'
sed -n '60,115p' iosMath/lib/MTMathList.m
printf '\n--- expansion call sites ---\n'
sed -n '1685,1785p' iosMath/lib/MTMathList.mRepository: kostub/iosMath
Length of output: 10012
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- macro definitions and template construction ---'
sed -n '1150,1210p' iosMath/lib/MTMathListBuilder.m
sed -n '1780,1825p' iosMath/lib/MTMathListBuilder.m
sed -n '1850,1890p' iosMath/lib/MTMathListBuilder.m
printf '%s\n' '--- static expansion-cycle verifier ---'
python3 - <<'PY'
from pathlib import Path
p = Path("iosMath/lib/MTMathList.m").read_text()
def body_after(signature, end_marker):
start = p.index(signature)
end = p.index(end_marker, start)
return p[start:end]
expand = body_after("- (MTMathList *)expandMacros", "`#pragma` mark NSCopying")
expansion = body_after("- (MTMathList *)expansion", "- (void)transferScriptsToExpansion")
checks = {
"expandMacros calls expansion": " expansion]" in expand or " expansion]]" in expand,
"expansion calls expandMacros": "[out expandMacros]" in expansion,
"expandMacros has no depth parameter": "AtDepth" not in expand and "depth" not in expand,
"expansion has no depth parameter": "AtDepth" not in expansion and "depth" not in expansion,
"no declared max depth symbol": "MaxMacroExpansionDepth" not in p,
}
for name, ok in checks.items():
print(f"{name}: {'yes' if ok else 'no'}")
if not all(checks.values()):
raise SystemExit("static expansion-cycle invariant failed")
PYRepository: kostub/iosMath
Length of output: 6976
Add a recursion bound to nested macro expansion.
-expandMacros calls -expansion, which calls -expandMacros again for nested arguments. No kMTMaxMacroExpansionDepth bound exists. Deeply nested input can exhaust the call stack.
🤖 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/MTMathList.m` around lines 1889 - 1890, Add a
kMTMaxMacroExpansionDepth guard to the recursive expandMacros/expansion flow,
tracking nested macro expansion depth and stopping or failing safely once the
bound is reached. Preserve normal expansion behavior for inputs within the limit
and ensure nested arguments cannot recurse indefinitely.
Source: Learnings
1b52511 to
3078e54
Compare
kostub
left a comment
kostub
left a comment
There was a problem hiding this comment.
Reviewed the seven [item 1]…[item 7] commits (b33eb3c..1b52511) that restore the #N template engine. Read-only review — no build, no test run. CI (Build & Test) is still IN_PROGRESS on 1b52511, so pass/fail is its answer, not mine.
What's solid: the swap is faithful to the plan and self-consistent — MTMacroParameterAtom's -copyWithZone: correctly reconstructs _argumentIndex after MTMathAtom's -initWithType:value: round-trip; -expansion deep-copies per placeholder so repeated #N and repeated -finalized both stay correct; _templateMode keeps # an invalid character in user input, and testBuildTemplateParsesParameterAtoms pins that. Dropping the finalized-loop assert (item 2) is safe — MTTypesetter.m:619-621 still catches the forged-type case. The test trim is defensible: the deleted unit tests each have a surviving end-to-end counterpart, the missing-argument rows live in MTMathListBuilderTest.m:1713-1719, and the equivalence tests still pin the three template constants against hand-written LaTeX, so a wrong template string still fails.
1. Stale selector in an exception message — iosMath/lib/MTMathList.m:197
reason:@"A macro atom cannot be created by type. Use -[MTMacroAtom initWithCommand:argument:prefix:suffix:] instead."Item 5 renamed that initializer to initWithCommand:arguments:templateExpression: and updated the sibling message in -[MTMacroAtom initWithType:value:], but missed this one. Trigger: any caller doing [MTMathAtom atomWithType:kMTMathAtomMacro value:@""] gets an exception pointing at a selector that no longer exists.
2. The LLD document is being committed into the repo — docs/lld/2026-07-13-modular-arithmetic.md
git ls-tree -r 1b52511 -- docs/ returns exactly this one file, and master tracks no docs/ directory at all — the rest of docs/ (plans, research, reqs, the other LLDs) is untracked working material. Item 7 adds 601 lines of internal design doc to the shipped repository. If that wasn't the intent, drop it from the branch; item 7's goal ("record the restore in the LLD") is satisfied by editing the untracked copy.
3. PR description contradicts itself
The opening paragraph still reads:
the design that emerged from their review had accumulated a
#Ntemplate engine, a placeholder atom type, and an expansion-depth budget that the three macros never needed. This is the same architecture the LLD specifies, built once, without that.
The "LLD fidelity" section directly below now says the #N template engine and the placeholder atom are exactly what this PR ships. The plan only called for replacing the "Deliberate deviation" section; the intro paragraph needs the same update.
4. MTMacroDefinition.argumentCount is a second source of truth for something the template already states
MTMathListBuilder.m — all three registry entries declare argumentCount:1 and reference #1. The arity is derivable from the template (max #N), and the only justification given is a future non-goal:
// Arity is declared rather than inferred from the template because a
// future \newcommand declares [argc] and its body may ignore arguments.\newcommand is an explicit non-goal in the LLD, so nothing today needs the two to be independent — and keeping them independent is what makes them able to disagree, which then has to be defended in two more places: the arity assert plus its release-mode fallback in -[MTMacroAtom expansion] (8 lines), and the arity half of testEveryRegisteredMacroParses.
Deriving it instead: the registry goes back to a plain NSDictionary<NSString*, NSString*>, the 22-line MTMacroDefinition class disappears (along with its redeclaration in the test file), and -expansion's bounds check becomes structurally unreachable rather than asserted. In -macroAtomForCommand:, parse the template first (it consumes no input, so the order swap is free):
NSString* templateString = [MTMathListBuilder builtinMacros][command];
if (!templateString) {
return nil;
}
MTMathList* templateExpression = [MTMathListBuilder buildTemplate:templateString];
// ...existing assert + internal-error guard...
NSUInteger argumentCount = 0;
for (MTMathAtom* atom in templateExpression.atoms) {
if ([atom isKindOfClass:[MTMacroParameterAtom class]]) {
argumentCount = MAX(argumentCount, [(MTMacroParameterAtom*)atom argumentIndex]);
}
}then the existing argument-reading loop, unchanged. Net: roughly −30 lines and one fewer way for the table to be wrong.
Separately, and only once because I gather this was settled on 2026-08-13: for three one-argument macros whose expansions are all prefix #1 suffix, the template engine (placeholder atom class + internal header + three build-system edits + _templateMode + the # parse branch) buys registry entries that read like the amsmath source, at the cost of two new failure modes the halves form couldn't have — arity/template disagreement, and #N nested in a sub-list silently not substituting. That's a real but small gain for the requirement as scoped. Your call; I'm not asking for a change.
5. iosMath.xcodeproj never gets lib/internal on its header search path
Package.swift added lib/internal to all three targets, but iosMath.xcodeproj/project.pbxproj:642-647 and :694-699 still list only render/internal — even though MTMathList.m and MTMathListBuilder.m now #import "MTMacroParameterAtom.h", which is not a sibling of either. Whether that compiles depends on the project headermap covering a header that sits in no build phase, which I can't establish by reading; CI's xcodebuild step will. If it's green, the point is only that this now diverges from how render/internal is wired — worth adding "$(SRCROOT)/iosMath/lib/internal" to both configurations for consistency.
1 participant
Rebuild of #265 + #266 as one PR. Those two were reverted in #267, and the rebuild first shipped a simpler prefix/suffix expansion in place of the LLD's
#Ntemplate engine. That call was reversed on review: the#Nengine and its placeholder atom are back, because arity-N table entries written in TeX macro syntax are worth the machinery. What did not come back is the expansion-depth budget and the recursive nested-placeholder walker — see "LLD fidelity" below.\bmodalready shipped in #264 as a symbol-table entry and is untouched here.What it does
\pmod{n},\mod{n},\pod{n}— one argument each, expanding to amsmath's exact inline form:\pmod{n}\mkern8mu(\mathrm{mod}\mkern6mu n)\mod{n}\mkern12mu\mathrm{mod}\mkern6mu n\pod{n}\mkern8mu(n)a \equiv b \pmod{n}now renders as it does in LaTeX.How
A registry entry is
{argumentCount, templateString}, where the template is ordinary LaTeX plus#Nargument references. An invocation parses to a singleMTMacroAtom— command name, parsed arguments, and the parsed argument-free template — and expands by splicing a deep copy of each argument into its#Nplaceholders.-[MTMathList finalized]expands every macro atom before the reclassifying pass, so:MTTypesetter.m);\pmod{a+}and friends agree with the written-out expansion — tested by comparing both against each other;\pmod{n}serializes back to\pmod{n}, not to its expansion.Scripts on the invocation transfer to the last script-capable atom of the expansion, so
\pmod{n}^2scripts the closing paren. On collision (\mod{n^2}^3) or with no target, both scripts land on a fresh empty Ordinary — the same fallback the builder already uses forx^2^3.LLD fidelity
This PR now ships the
#Ntemplate engine LLD §3.1/§3.3 specify (restored after review): a registry entry is{argumentCount, templateString}and the template is written in TeX macro syntax, so table entries read exactly like the amsmath definitions they reproduce and extend to any argument count. Substitution reaches only the top level of a template —#Ninside\frac{},{…}, or a script is not expressible. That, and the expansion-depth budget, remain\newcommand-era work (LLD §8.2).Error handling
MTParseErrorMissingArgument— a command that requires an argument and is given none. Covers end of input, a}/^/_/&in argument position, and stop commands (\right,\\,\cr,\end,\over, …), which would otherwise terminate the enclosing list and be handed back as the "argument" — silently losing a matrix row for\begin{matrix}a\pmod\\b\end{matrix}. Only macros route through the new check;\sqrtkeeps its long-standing permissive behavior.Known gap
amsmath widens the leading gap to 18mu in display style (
\if@display). iosMath can't: a macro expands at parse time, before the render style is known, so it always emits the inline 8mu/12mu. Out of scope per PRD §3.1/§9.1.Testing
swift test— 482 tests, 0 failures (43 inMTModularArithmeticTest, trimmed to the set that catches a real regression).swift buildclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Summary by CodeRabbit
New Features
\bmod,\pmod,\mod, and\pod.Documentation
Tests