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
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds deferred macro atoms and built-in support for ChangesModular arithmetic macros
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Input
participant MTMathListBuilder
participant MTMacroAtom
participant MTMathList
participant MTTypesetter
Input->>MTMathListBuilder: parse modular-arithmetic command
MTMathListBuilder->>MTMacroAtom: construct macro with arguments and prefix/suffix
MTMathList->>MTMacroAtom: expand during finalization
MTMacroAtom-->>MTMathList: return expanded atom sequence
MTMathList->>MTTypesetter: typeset finalized sequence
Possibly related PRs
🚥 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.
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
1 participant
Rebuild of #265 + #266 as one PR. Those two were reverted in #267 — 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.\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
An invocation parses to a single
MTMacroAtom— command name, parsed arguments, and the fixed prefix/suffix LaTeX bracketing them.-[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.Deliberate deviation from the LLD
LLD §3.1/§3.3 specify a
#Ntemplate engine. This ships prefix/suffix halves instead: two ordinary LaTeX strings parsed by the ordinary builder, no template mode, no#Nparsing, no placeholder atom type. For an expansion with one substitution region — all three of these — it is exactly equivalent, and it removes ~230 production lines along with both template-invariant asserts. LLD §4.1 already records why: the engine's stated payoff was\newcommandreuse, but\newcommand{\abs}[1]{\left|#1\right|}needs substitution that descends into sub-lists, which a top-level template engine cannot do either way.The cost: an expansion interleaving fixed text between several arguments isn't expressible. None of the three needs that, and it's noted in the header doc.
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— 526 tests, 0 failures (87 inMTModularArithmeticTest).swift build -c releaseclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw
Summary by CodeRabbit
New Features
\bmod,\pmod,\mod, and\pod.Documentation
Tests