migration to seq 6.x #75

Merged

navicore merged 1 commit from seq-6 into main

2026-04-28 17:19:27 +00:00

navicore commented

2026-04-28 16:34:01 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-04-28 16:38:49 +00:00

(Migrated from github.com)

Code Review: migration to seq 6.x

Overall this is a well-executed, large-scale mechanical migration. The transformations are consistent, the design doc is clear, and the scope is appropriately constrained.

What is done well

Design doc is excellent. docs/design/seq-6.0-if-combinator-migration.md clearly states the two transformation rules, out-of-scope items, and checkpoints. The FOLLOWUP: pattern for deferring future refactors is a good discipline.
Mechanical consistency. All cond if A else B then to cond [ A ] [ B ] if and cond if A then to cond [ A ] when transformations look correctly applied throughout all 11 files.
Good use of unless. The emit-token rewrite in tokenizer.seq correctly uses unless where the original had an empty true-branch, which is more idiomatic than [ ] [ work ] if.

Minor observations

1. include std:control placement (src/sexpr.seq)

Only sexpr.seq adds include std:control. This works if sexpr.seq is transitively included by every file that now uses when/unless/combinator if. Worth confirming the include chain is complete. If any file pulls in a different root module that omits std:control, a build failure could appear on specific paths.

2. Removed comment in emit-token

The original code had a meaningful comment in the empty true-branch; the ] unless form dropped it. Since unless is a logical inversion (quotation executes when condition is false), a brief comment helps readers unfamiliar with the combinator. Something like No-op if token empty; otherwise emit before the [ would restore the context.

3. Empty true-branch in tokenize-loop

The [ ] empty quotation for the base case is correct. State is already on the stack when done; no additional work needed. This matches the design doc pattern for divergent recursive base cases.

4. Dispatch chain style

The multi-level chains use ] [ dup X string.equal? [ inlined on the same line throughout eval.seq, consistently. Restructuring is explicitly out of scope for this PR, so fine as-is.

Spot-check correctness

int-abs: dup 0 i.< [ 0 swap i.subtract ] when -- correct one-armed form
min: 2dup i.> [ nip ] [ drop ] if -- correct two-armed form
trev-loop base case via [ drop ] in the true branch -- correct
format-eval-error nested closing ] if # Close guard if-else -- nesting aligns correctly

CI

Tests are in progress at review time. The key behavioral risk is TCO: recursive tail calls inside quotations need musttail lowering under seqc 6.1.0. The design doc says this was verified with patch-seq, but the 10k-depth countdown smoke test (checkpoint 4) is the definitive confirmation.

Summary

Clean, well-scoped migration. Actionable items: (1) confirm include std:control transitive coverage across all migrated files, and (2) optionally restore a comment on the emit-token unless line. Everything else is a nit. Good work keeping the diff purely syntactic.

## Code Review: migration to seq 6.x Overall this is a well-executed, large-scale mechanical migration. The transformations are consistent, the design doc is clear, and the scope is appropriately constrained. ### What is done well - **Design doc is excellent.** `docs/design/seq-6.0-if-combinator-migration.md` clearly states the two transformation rules, out-of-scope items, and checkpoints. The `FOLLOWUP:` pattern for deferring future refactors is a good discipline. - **Mechanical consistency.** All `cond if A else B then` to `cond [ A ] [ B ] if` and `cond if A then` to `cond [ A ] when` transformations look correctly applied throughout all 11 files. - **Good use of `unless`.** The `emit-token` rewrite in `tokenizer.seq` correctly uses `unless` where the original had an empty true-branch, which is more idiomatic than `[ ] [ work ] if`. ### Minor observations **1. `include std:control` placement (`src/sexpr.seq`)** Only `sexpr.seq` adds `include std:control`. This works if `sexpr.seq` is transitively included by every file that now uses `when`/`unless`/combinator `if`. Worth confirming the include chain is complete. If any file pulls in a different root module that omits `std:control`, a build failure could appear on specific paths. **2. Removed comment in `emit-token`** The original code had a meaningful comment in the empty true-branch; the `] unless` form dropped it. Since `unless` is a logical inversion (quotation executes when condition is false), a brief comment helps readers unfamiliar with the combinator. Something like `No-op if token empty; otherwise emit` before the `[` would restore the context. **3. Empty true-branch in `tokenize-loop`** The `[ ]` empty quotation for the base case is correct. `State` is already on the stack when done; no additional work needed. This matches the design doc pattern for divergent recursive base cases. **4. Dispatch chain style** The multi-level chains use `] [ dup X string.equal? [` inlined on the same line throughout `eval.seq`, consistently. Restructuring is explicitly out of scope for this PR, so fine as-is. ### Spot-check correctness - `int-abs`: `dup 0 i.< [ 0 swap i.subtract ] when` -- correct one-armed form - `min`: `2dup i.> [ nip ] [ drop ] if` -- correct two-armed form - `trev-loop` base case via `[ drop ]` in the true branch -- correct - `format-eval-error` nested closing `] if # Close guard if-else` -- nesting aligns correctly ### CI Tests are in progress at review time. The key behavioral risk is TCO: recursive tail calls inside quotations need `musttail` lowering under seqc 6.1.0. The design doc says this was verified with patch-seq, but the 10k-depth countdown smoke test (checkpoint 4) is the definitive confirmation. ### Summary Clean, well-scoped migration. Actionable items: (1) confirm `include std:control` transitive coverage across all migrated files, and (2) optionally restore a comment on the `emit-token` `unless` line. Everything else is a nit. Good work keeping the diff purely syntactic.