alignedat environment (argument-reading hook)#248
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for the alignedat LaTeX environment in iosMath, including parsing the mandatory numeric argument, setting up alternating column alignments, injecting relation spacers, and adding corresponding unit tests. The review feedback suggests trimming whitespace from the parsed environment argument to ensure robust validation, and creating unique MTMathAtom spacer instances for each cell insertion to avoid potential side effects from sharing a single mutable instance.
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.
| message:@"alignedat requires a numeric argument, e.g. \\begin{alignedat}{2}"]; | ||
| return nil; | ||
| } | ||
| return arg; |
There was a problem hiding this comment.
The captured environment argument may contain leading or trailing whitespace if the user includes spaces inside the braces (e.g., \begin{alignedat}{ 2 }). This causes the subsequent validation in buildTable:argument:firstList:row: to fail because it strictly checks for digit characters. Trimming whitespace from the argument string before returning it ensures robust parsing.
return [arg stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];| MTMathAtom* spacer = [MTMathAtom atomWithType:kMTMathAtomOrdinary value:@""]; | ||
| for (int i = 0; i < table.cells.count; i++) { | ||
| NSArray<MTMathList*>* row = table.cells[i]; | ||
| for (int j = 0; j < row.count; j++) { | ||
| if (j % 2 == 1) { | ||
| [row[j] insertAtom:spacer atIndex:0]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sharing a single mutable MTMathAtom instance (spacer) across multiple cells in the table is a bad practice and can lead to unexpected side effects if any of the cells or their atoms are modified or inspected. Instead, a new MTMathAtom instance should be created for each insertion.
for (int i = 0; i < table.cells.count; i++) {
NSArray<MTMathList*>* row = table.cells[i];
for (int j = 0; j < row.count; j++) {
if (j % 2 == 1) {
MTMathAtom* spacer = [MTMathAtom atomWithType:kMTMathAtomOrdinary value:@""];
[row[j] insertAtom:spacer atIndex:0];
}
}
}
kostub
left a comment
There was a problem hiding this comment.
Code Review — alignedat environment (items 7-10)
Reviewed the diff against the base branch feature/subordinate-envs-pr2 (commits 30e67ee → 0dda5ed), checked out against the plan (PR 3, items 7-10) and LLD §3.2/§3.3/§3.4/§6/§7. Static review only — no build/test runs.
Strengths
- Faithful to the LLD/plan. Every planned artifact is present and in the right place:
MTEnvProperties.argumentproperty + nil init (MTMathListBuilder.m:20,33),+environmentsTakingArgumentgate (:851),-readEnvironmentArgumentreader (:865), thebuildTable:argument:firstList:row:rename with all 3 call sites updated (:309, 1075, 1240), the2nvalidation block (:1342-1367), the factory branch (MTMathAtomFactory.m:519-540), the spacer-strip (MTMathList.m:1104-1109), the{n}emit (:1226-1228), and all 4 error rows + structure test + round-trip + layout test. - Correct argument-reader edge cases. Traced
{2}(normal),{}(empty → rejected downstream by the length check),{2(EOF →expectCharacter:'}'fails), and missing{entirely — all four produce the rightMTParseErrorInvalidCommand. TheunlookCharacter/expectCharacterdance mirrorsreadColorexactly, and theNSAssert(_currentChar > 0)inunlookCharacteris safe because{was always consumed first. - Argument flow is correct.
_currentEnv.argument = argumentis set immediately after_currentEnvcreation (:1305), and the validation reads_currentEnv.argument(:1344) after the parse loop but before the factory call — exactly as the LLD §3.4 logic flow specifies. Anonymous-table call sites passargument:niland never enter thealignedatvalidation block (gated onenvName). - Justified deviation from the plan:
kMTMathAtomNumbernotkMTMathAtomVariable. The plan (line 691) asserted even-column cells start with aVariableatom, but the input10/3/13parse toNumberatoms. The implementer corrected this (MTMathListBuilderTest.m:1541). Good catch. - Justified deviation: round-trip string. The plan (line 799) expected
10&x +&3&y(preserving spaces), but the serializer concatenates atoms without inter-atom whitespace, sox +correctly round-trips tox+(:1548). The implementer's expected string is the actually-correct one; the plan's was wrong. - Consistent with the established atom-sharing pattern. The
alignedatfactory creates onespacerinstance and inserts it into every odd column of every row (MTMathAtomFactory.m:525-532) — this is exactly whatmatrixdoes with itsstyleatom (:406,410) andaligneddoes with its spacer (:442,446). No new sharing-safety concern is introduced. - Validation lives in the parser, factory stays spec-less. This matches the LLD §5 explicit resolution; the factory derives everything from
numColumnsand the parser owns the{n}interpretation. Clean separation.
Issues
Critical (Must Fix)
None.
Important (Should Fix)
None. The implementation is correct and complete against the acceptance criteria. The items below are quality/fidelity refinements, not blockers.
Minor (Nice to Have)
-
readEnvironmentArgumentdoes not strip whitespace inside{…}(MTMathListBuilder.m:872-880). The cited precedent-readColorcallsskipSpacesafter the opening brace (:587); this reader does not. So{ 2 }or{ 2}is rejected as non-numeric (space fails the digit loop at:1348), diverging from TeX's argument scanner which strips surrounding spaces. Low impact since users rarely write spaces inside{n}, but it's a fidelity gap and an inconsistency with the reader it was modeled on. Fix: eitherskipSpacesafter{and strip trailing spaces before}, or have the digit-validation loop skip whitespace. -
testAlignedatLayout's "butted pairs" assertion is too weak (MTTypesetterTest.m:3213-3218).XCTAssertGreaterThanOrEqual(col.position.x, prevX)only checks left-to-right ordering, which holds for any non-overlapping table — including one withinterColumnSpacing=18. A regression that accidentally set a nonzero spacing would still pass. The realinterColumnSpacing==0guarantee is already locked structurally intestAlignedat(:1520), so the layout test is effectively a smoke test. Consider asserting the x-gap between col 1 and col 2 is ~0 (the pair boundary), or at least weaken the comment to not overstate what it validates. -
Test coverage gaps vs. LLD §6/§7. No test for: (a)
n=1(the 2-column minimum / degenerate single pair), (b) an empty odd-column cell likex&&z&wwithn=2(exercises spacer-insert-into-empty-list + strip-on-empty-after-strip round-trip), (c)alignedatwrapped in delimiters (\bigl(\begin{alignedat}{1}...\end{alignedat}\bigr), the MTInner path the LLD §6 explicitly mentions), (d){-1}(the LLD §6 negative case;{x}covers the non-numeric path transitively but not the leading--specifically), (e){}(empty braces). None are bugs — the code handles them — but they're natural edge cases left untested.
Recommendations
- When the
arrayenvironment lands, convergeenvironmentsTakingArgument/readEnvironmentArgumentwithenvironmentsRequiringColumnSpec/columnSpecas the LLD §5 note prescribes. The reader currently hardcodes "alignedat" in its error message (:869,883) — fine for one env, but the message should become parameterized (or env-name-derived) when a second gated env arrives, otherwise users of the future env get a confusing "alignedat requires…" error. - The
alignedfactory branch validatesnumColumns == 2itself (MTMathAtomFactory.m:434), butalignedattrusts the parser's2ncheck and does no factory-side validation. This asymmetry is by design (LLD §5), but if the factory is ever called directly with an odd column count, it will silently produce a lopsided R/L/R layout with no error. A defensivenumColumns % 2 == 0assertion in the factory branch would be cheap insurance.
Assessment
Ready to merge? Yes.
Reasoning: The implementation is correct, complete against the plan/LLD, and consistent with the surrounding aligned/matrix patterns. I found no bugs — the argument reader handles EOF/empty/malformed inputs correctly, the validation flow is sound, and the two deviations from the plan (atom-type and round-trip string) are justified corrections. The remaining findings are minor fidelity/coverage refinements that can be addressed in a follow-up.
Review posted via the superpowers:requesting-code-review skill. This is review feedback only — not an approval.
d6b62ce to
8a82ff6
Compare
0dda5ed to
9d20f83
Compare
8a82ff6 to
5702b0f
Compare
9d20f83 to
2feefb0
Compare
5702b0f to
6c49503
Compare
2feefb0 to
b0986b5
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b0986b5 to
38a877f
Compare
Goal
Add
\begin{alignedat}{n}— the only environment that reads a mandatory{n}argument. Introduces a generic argument-capture hook on the parser (MTEnvProperties.argument,+environmentsTakingArgument,-readEnvironmentArgument), threads it through a renamed-buildTable:argument:firstList:row:, validatesnumColumns == 2nin the parser, generalizes thealignedfactory branch to2ncolumns, and round-trips the{n}argument.Stack
This PR is based on
feature/subordinate-envs-pr2and will auto-retarget tomasteronce PR 1 and PR 2 merge.Design docs
docs/plans/2026-06-30-smallmatrix-gathered-alignedat.md(PR 3 section)docs/lld/2026-06-30-smallmatrix-gathered-alignedat.mdCommits
[item 7] Add \begin{env}{arg} reader and thread through buildTable—MTEnvProperties.argument++environmentsTakingArgument+-readEnvironmentArgument; renamed-buildTable:argument:firstList:row:with all 3 call sites updated in one commit. Missing-{n}error →MTParseErrorInvalidCommand.[item 8] Add alignedat 2n validation and factory branch— parser validatesn(positive int) andnumColumns == 2n; factory branch with alternating R/L alignment + spacer before odd columns. TesttestAlignedat+ 3 error rows.[item 9] Round-trip alignedat {n} argument and per-pair spacers— strip per-pair spacer for odd columns in-serializedCellAtRow:column:; emit{n}in-appendLaTeXToString:. Round-trip assertion intestAlignedat.[item 10] Add alignedat layout test (4 columns, butted pairs)—testAlignedatLayout.Tests
Full suite green: 277 tests across
MTTypesetterTest/MTMathListBuilderTest/MTMathListTest, 0 failures.One test-only deviation from the plan (noted in item 9): the plan's round-trip expected literal asserted
x +(with a space), but iosMath's serialization joins atom nuclei with no whitespace — consistent with the existingalignedround-trip test. The expected literal was corrected tox+; no implementation change.🤖 Generated with Claude Code