diff --git a/MAINTENANCE.md b/MAINTENANCE.md index 8fa8886..a2812a3 100644 --- a/MAINTENANCE.md +++ b/MAINTENANCE.md @@ -201,7 +201,7 @@ compile-time test `libakstdlib` could not write against `libakerror` — and `ak floor is enforced instead by an `#error` feature-testing `AKERR_FIRST_CONSUMER_STATUS`, which `akstdlib.h`, `akgl/error.h` and our own `include/akbasic/error.h` all carry — include any of them and you inherit the guard. The missing version file is filed in -`deps/libakstdlib/TODO.md` §2.3; when it lands, add the `1.0` floor to the `find_dependency` +`libakstdlib` issue #5 and `libakerror` issue #16; when it lands, add the `1.0` floor to the `find_dependency` calls. ### Embedding all three dependencies collides four ways @@ -253,7 +253,7 @@ target with the same name already exists"*. We shadow `add_custom_target` and re to `akerror_coverage` on the way past. `libakstdlib` (both targets) and `libakgl` (its `mutation` target) namespace themselves correctly. **The real fix is upstream in `libakerror`** — the same `CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR` test it already -applies to `mutation` — and it is filed in `deps/libakstdlib/TODO.md` §2.3. Delete the +applies to `mutation` — and it is filed as `libakerror` issue #15. Delete the workaround when it lands. **4. Stale build trees poison the coverage report.** See below; it is the reason for diff --git a/TODO.md b/TODO.md index b1c8934..f52a2f1 100644 --- a/TODO.md +++ b/TODO.md @@ -1,11 +1,24 @@ -# TODO +# Record -Implementation plan for the Go → C port of `deps/basicinterpret`. +**Outstanding work is in the issue tracker, not in this file:** + -This file is written to be executed by AI agents, not read by humans for inspiration. Every -item names the files it touches, the exact deliverable, and the command that proves it done. -Work the phases in order. Inside a phase, items with no `Depends:` line may be done in -parallel. +This file was the implementation plan for the Go → C port of `deps/basicinterpret`, written to be +executed by AI agents rather than read for inspiration. **The port is done**, so what it is now is +the record: the design decisions that are settled, the deviations from the reference and why each +was taken, the defects that were found and fixed, and the reasoning behind the measurements. + +Each open item that moved leaves a line saying what it was and which issue carries it, because an +entry explaining *why* a defect matters is worth keeping beside the work it constrains — but the +tracking happens there, not here. + +Issues are labelled by kind and blast radius, and milestoned by what they can land in: `0.1.x` for +anything that changes no public contract, `0.2.0` for new verbs and observable behaviour changes, +`1.0.0` for the design decisions. Everything filed carries `status::grooming`. + +Two gaps this file recorded and deliberately did not file — the missing `HUD` anchors and a +dismissable dialog — are now `libakgl` #79 and #80. §7's rule is right that *changing* a dependency +is that repository's decision; it does not follow that reporting the gap is. --- @@ -44,7 +57,7 @@ What it does **not** change: 2. `deps/libakerror/AGENTS.md` — the `ATTEMPT`/`CLEANUP`/`PROCESS`/`HANDLE`/`FINISH` protocol. 3. `deps/libakerror/UPGRADING.md` — 1.0.0's status registry. Required before writing an error code; the mechanism it replaced is gone. -4. `deps/libakstdlib/TODO.md` §2.1 — six **confirmed** defects in the library this port calls +4. `deps/libakstdlib`'s issue tracker — the defects and gaps in the library this port calls into. §1.9 below says which calls are cleared for use; that section is not optional reading, it bans a family of functions the port would otherwise reach for by reflex. 5. `deps/libakgl/AGENTS.md` — the no-`malloc` rule and the commit co-author requirement. @@ -55,16 +68,17 @@ What it does **not** change: - **Do not** mark an item done until its acceptance command passes on a clean out-of-tree build. "It compiles" is not acceptance. -- **Do** update this file in the same commit as the work: strike the item, or replace it with - the defect it uncovered. This file holds outstanding items only. +- **Do** update the tracker in the same commit as the work: close the issue, or replace it with + the defect it uncovered. Outstanding items live there; this file holds the record. - **Do** add the agent program name, model name and version as a commit co-author. That rule comes from `libakgl` and applies here. - **Never** hand-edit generated output. `build/` trees and the generated `akerror.h` are off-limits; change the generator. - **Never** reformat a file you are not otherwise changing. - When a step is blocked because `libakgl` cannot supply a capability, **do not work around - it here.** File it in `deps/libakgl/TODO.md` in the numbered prose style of that file's - "Carried over" section and note the block in §7 below. + it here.** File it against `libakgl` in its issue tracker -- what the BASIC verb requires, + what the `akgl_*` entry point should look like, and what tests would cover it -- and note the + block in §7 below. **Style**, restated so nobody has to go look: C99, 4-space indent, tabs at width 8 (`stroustrup`), function-body braces in column 0 on their own line, control braces on the same @@ -140,7 +154,7 @@ One implementation, `src/symtab.c`, keyed by `aksl_strhash_djb2()` (already in `libakstdlib`) with linear probing and a fixed slot array. **Use the existing hash; do not write another one.** Table full is an error, not a resize. -Caveat, from `deps/libakstdlib/TODO.md` §1.6/§2.2.6: the wrapper sign-extends `char`, so a +Caveat, recorded upstream: the wrapper sign-extends `char`, so a high-bit byte hashes wrong — `"\xff\xfe"` returns 5859874 where the `unsigned char` answer is 5868578. BASIC identifiers are 7-bit ASCII (the scanner only accepts `IsLetter`/`IsDigit` plus a type suffix), so this cannot bite the symbol tables. It **would** bite if anyone later keys @@ -729,81 +743,21 @@ Also on the queue, from the reference's own defect list: ### What a demoscene program wanted and could not have -`examples/megademo/megademo.bas` was written to push the interpreter to its edges, and these -are the edges it hit. None of them blocked the demo — every one has a workaround, and the -program carries all six — but each workaround is a routine every future game will carry too, -and that is the argument for the verb. Ordered by how much BASIC each one would delete. +`examples/megademo/megademo.bas` was written to push the interpreter to its edges, and these are +the edges it hit. **None of them blocked the demo** — every one has a workaround and the program +carries all six — but each workaround is a routine every future game will carry too, and that is +the argument for the verb. -- **`RND` is still missing.** Chapter 17 teaches the LCG workaround and the demo's `RANDOM` - label carries it: nine tokens, two convention globals (`RMAX#` in, `RND#` out), and a seed - the caller must remember to take from `TI#`. That is the right *teaching* example and the - wrong thing to make every program transcribe. Closing it is the standard function shape — - dispatch-table row, exec handler, tests, a Chapter 12 row — and Chapter 17's section becomes - a history lesson instead of a requirement. `ASC` is in the same boat with a worse workaround - (`INSTR` into a 41-character alphabet string); `INT` and `SQR` are not, because integer - assignment and `^ 0.5` already are those functions. -- **The palette cannot be written.** `PALETTE[17]` in `src/graphics_tables.c` is `static - const`, and `COLOR` can only choose among its sixteen entries. Palette rotation was the - cheapest animation of the whole era — one register write cycled every pixel of a colour — - and here the same effect is a full redraw: the demo's title "colour cycle" re-strokes the - logo every frame (`DRAWTEXT` with `CM# = 1`), and its raster bars erase and redraw five - ramp lines apiece per frame. Closing it means palette state moves from a static table to - the graphics backend, plus a verb form to set an index (`COLOR` grew nothing new on a C128, - so this would be ours — Chapter 13 material), plus the docs figures that assume Pepto's - values stay put. -- **`GSHAPE` stamps in one mode.** BASIC 7.0's takes a mode argument — replace, OR, AND, - XOR — and XOR is the one that mattered: stamp to draw, stamp again to erase, no damage to - what was underneath. That is software sprites beyond the eight real ones, and it is the - difference between the demo's raster bars chewing holes in the starfield (they erase in - black) and leaving it intact. Needs a blend argument through `SSHAPE`/`GSHAPE`'s render - path in `src/graphics_akgl.c`, and libakgl may need to offer the blend mode first — check - before filing there. -- **Nothing reports whether `PLAY` has drained.** The queue depth exists in - `akbasic_AudioState` (`count - head`); no function reads it. The demo's `TUNE` label - requeues each batch on a wall-clock guess — it stamps `MT#` with the batch's computed - length plus a breath, and the sums drift against `TEMPO` and had better stay under the - queue's 128-note cap when a requeue lands early. An `RPLAY(v)` returning notes pending — - same shape as `RSPPOS` — deletes the arithmetic. It also fixes the quiet failure Chapter 7 - warns about, where a program `QUIT`s before its music finishes and never knows. -- **`PLAY` is monophonic: one queue, one clock, one instrument per batch.** An earlier - version of this entry claimed there was no noise waveform, and that was wrong — - `ENVELOPE`'s sixth argument selects triangle, sawtooth, square or noise - (`src/runtime_audio.c`), which Chapter 7 does not document and should. The real gap is in - `akbasic_play_service()` (`src/play.c`): every voice's notes go into **one** queue with one - release clock, and the envelope applied when a note sounds is whatever the last `T` - *parsed* selected — so a program that writes three `V` tracks gets them end to end, on one - instrument, and `M` is a no-op precisely because there is nothing to synchronise. The - backend already has three independent voices and `tone()` takes a voice number; what is - wanted is a queue head and clock per voice, and the envelope resolved at parse time into - the note rather than read from shared state at release. The demo works around it the way - 1985 did on one channel: an interleaved kick–bass–arpeggio line (the `TUNE` label), which - is authentic and should not be mandatory. -- **`DATA` is too small a pipe for assets, and `SPRSAV`'s type-in form is 24 by 21.** The - pool is 512 items and the demo's stroke font already holds ~350, so a full-screen picture - can never arrive by `DATA` — and the one sprite form that reads from the program is fixed - at the VIC-II's 24x21 monochrome. The demo's scene 5 shows both the ceiling and the way - around it: a 160x120 sixteen-colour image rides in **string literals** instead — RLE, two - characters a run, ~4 KB in eighteen `IM$()` assignments, decoded with `INSTR` and painted - in 5x5 blocks (`examples/megademo/vaporwave.py` generates the block) — while the 63-byte - bird beside it is all `DATA` can actually carry. Strings are the workaround, not the - answer: every program that wants a picture must also carry a decoder. What is wanted is - either a bigger `DATA` pool (it is one constant) or, better, a `SPRSAV`/`BLOAD` form that - accepts packed image data from program text at a stated width and height. -- **The only vsync is spinning on `TI#`, and the spin is worse than it looks.** Chapter 13 - documents the polling; the demo's first cut had the loop — read `TI#`, branch to yourself - until it moves — and it did not just waste CPU, it **starved the `PLAY` queue dry**. The - chain is measurable: the host calls `settime()` once per `runtime_run()` batch, and - `akbasic_play_service()` can therefore release at most one note per batch (after a release, - `nextms` sits ahead of the frozen clock). A batch of 256 *held* `SLEEP` steps is nearly - free, but a batch of 256 *executed* spin statements is slow enough to drop the release rate - below a sixteenth-note tune's fifteen notes a second — and the queue never reclaims slots - until it drains completely, so the backlog compounded across scenes and overflowed the - 128-note queue with `PLAY queue is full`. Reproduced both ways in isolation: the same tune - requeued eight times over a `SLEEP` loop survives, over a `TI#` spin it dies on the third - batch. The demo's `VSYNC` label now sleeps a frame instead. A verb that parks the program - until the next batch boundary (`SLEEP 0` is an available spelling) is the same - hold-without-blocking shape `SLEEP` and `GETKEY` already have in `src/runtime_console.c`, - and it would make the honest spelling also the safe one. +Ordered by how much BASIC each one would delete: `RND` and `ASC` (#16), the unwritable palette +(#17), `GSHAPE`'s missing blend modes (#18), no way to ask whether `PLAY` has drained (#19), +`PLAY` being monophonic (#20), `DATA` being too small a pipe for assets (#21), and the `TI#` spin +that starves the audio queue (#22). + +**The last of those is the one worth reading before writing a game loop.** Polling `TI#` does not +merely waste CPU: the host calls `settime()` once per `runtime_run()` batch, so a batch of spin +statements drops the note release rate below what a sixteenth-note tune needs, the queue never +reclaims slots until it drains completely, and the backlog compounds until `PLAY queue is full`. +Reproduced both ways in isolation. Sleep a frame instead. --- @@ -1259,7 +1213,7 @@ deviations from the reference's *program*: `main.go` and the SDL half of 57. **`DIRECTORY` is refused for want of an upstream wrapper.** `libakstdlib` has no `aksl_opendir`, and this project's rule is that a missing capability is filed upstream - rather than worked around — so it is filed in `deps/libakstdlib/TODO.md` and the verb says + rather than worked around — so it is filed against `libakstdlib` and the verb says so. Calling `opendir(3)` here would mean reporting through `errno` in a file where everything else reports through an `akerr_ErrorContext *`. @@ -1692,28 +1646,11 @@ the reference's semantics reproduced faithfully; the third is the port's own. zero-iteration body uses, and the `NEXT` pops on an `exiting` flag. Neither verb has to know where the loop ends. `tests/for_next.c`. -19. **A `FOR` body runs once with the overshot counter.** The loop condition is tested against - the counter *before* the increment, so the increment's result reaches the body before - anything compares it to the limit. `FOR I = 1 TO 9 STEP 3` runs its body with 1, 4, 7 **and - then 10**. Invisible whenever the step lands exactly on the limit, which is every case in - the golden corpus and every case in the reference's own tests. - - Related, and the same off-by-one from the other end: `FOR I = 1 TO 1` does not run its body - at all, where every BASIC ever written runs it once. The two errors compensate for a step of - 1, which is why neither was noticed. - - **Not fixed, because the corpus pins it.** - `tests/reference/language/flowcontrol/forloopwaitingforcommand.bas` depends on - `FOR I# = 1 TO 1` skipping its body, and `tests/reference/README.md` forbids editing an - expectation to suit this interpreter. Registered as `tests/for_semantics.c` in - `AKBASIC_KNOWN_FAILING_TESTS`, asserting the correct contract. Fixing it means deciding what - to do with that golden file, and that is a decision rather than a patch. - -20. **A `FOR` counter does not survive its loop.** It is created in the environment the loop - pushes, which pops when the loop ends, so reading it afterwards finds a fresh variable - holding zero. A C128 leaves it at the value that ended the loop and plenty of published - listings read it there. Same known-failing test as item 19; the fix is a scoping decision - about where a loop counter is created, not an ordering one. +19. **A `FOR` body runs once with the overshot counter**, `FOR I = 1 TO 1` does not run its + body at all, and the counter does not survive the loop. The first two compensate for a step + of 1, which is why neither was noticed. Pinned by `tests/for_semantics.c` in + `AKBASIC_KNOWN_FAILING_TESTS`; fixing it means deciding what to do with + `forloopwaitingforcommand.bas`, which depends on the current behaviour. **Issue #5.** ### Found while fixing the DEF recursion hang @@ -1756,31 +1693,10 @@ the reference's semantics reproduced faithfully; the third is the port's own. A *structure* is deliberately given no truth value -- it always exists, so the question has no answer worth guessing at. -25. **A statement containing a failed multi-line `DEF` call still completes, and prints a - junk value.** The call's error is reported and the run stops, but the *enclosing* - statement carries on with whatever `*dest` was left holding: - - ``` - 10 DEF BAD(N#) - 20 RETURN N# / 0 - 30 PRINT 99 - 40 PRINT BAD(1) - ``` - - prints `99`, the division-by-zero line, and then - `(UNDEFINED STRING REPRESENTATION FOR 0)`. - - **Pre-existing, and measured as such**: identical before and after the re-entrancy - fix, on a build stashed back to compare. It is visible more often now only because - runaway recursion reaches it where it used to hang instead. - - The cause is that `akbasic_runtime_process_line_run()` swallows a script's error by - design -- goal 3 -- so the loop inside `akbasic_runtime_user_function()` sees the run - end normally and hands back a value nobody should use. The fix is for the multi-line - path to notice the run did not return through `RETURN` and refuse rather than produce - a value; it is not done here because it is a decision about what a failed call - *evaluates to*, and `tests/language/functions/recursion.bas` deliberately does not - pin the current answer. +25. **A statement containing a failed multi-line `DEF` call still completes, and prints a junk + value.** `akbasic_runtime_process_line_run()` swallows a script's error by design, so the + loop inside `akbasic_runtime_user_function()` sees the run end normally. It is a decision + about what a failed call *evaluates to*. **Issue #7.** ### Found while building structures @@ -1863,111 +1779,38 @@ the reference's semantics reproduced faithfully; the third is the port's own. ### Found while writing the error-code appendix 21. **A dependency's status code reaches `ER#` where the interpreter has one of its own.** - `VAL("XYZ")` reports `AKERR_VALUE` — 144 on this machine — rather than - `AKBASIC_ERR_VALUE` (517), because `akbasic_fn_val()` (`src/runtime_functions.c:261`) - hands the string to `aksl_atof()` and returns whatever comes back. Both register the - name `Value Error`, so `ERR(ER#)` cannot tell them apart and only the number can. - - **The consequence is that the number is not portable.** `libakerror`'s codes are offsets - from the platform's largest `errno`, so a program testing `IF ER# = 144` is right on one - machine and wrong on the next, where 517 — which is absolute — is what it should have - been able to test. `docs/15-error-codes.md` documents the behaviour as it stands and - tells a program to compare the code rather than the text. - - Not fixed here because it is a decision about a boundary rather than a patch: every - place the interpreter calls into `libakstdlib` on a *program's* behalf could translate - the failure into the akbasic band, and the list of those places wants collecting before - the first one is changed. The disk verbs are the interesting counter-case — `ENOENT` out - of `DOPEN` is genuinely more useful to a program than a flat 519 — so this is not - "translate everything", which is exactly why it needs deciding rather than doing. + `VAL("XYZ")` reports `AKERR_VALUE` rather than `AKBASIC_ERR_VALUE`, and the errno-derived + number is not portable across machines where 517 would have been. A decision about a + boundary rather than a patch -- the disk verbs are the counter-case, since `ENOENT` out of + `DOPEN` is more useful than a flat 519. **Issue #13.** ### Found while making line numbers optional -26. **`akbasic_environment_set_label()` files into the active scope, not the root.** - `src/environment.c:246` walks up until `obj->runtime->environment == obj`, which is the - *currently executing* scope, despite the comment above it saying "Only the top-level - environment creates labels". So a `LABEL` reached inside a `GOSUB` or `FOR` body is filed - into that scope and dies when the body pops, while `akbasic_runtime_scan_labels()` - (`src/runtime.c:1286`) files the same label into the real root. +26. **`akbasic_environment_set_label()` files into the active scope, not the root**, so a + prescanned label and a re-filed one behave differently. One condition, plus a test. Wants + checking against deviation 32 first. **Issue #9.** - **The consequence is that a prescanned label and a re-filed one behave differently**, and - which one you get depends on whether control has passed through the `LABEL` statement - inside a scope. Nothing in either corpus does that, so nothing catches it. The fix is one - condition -- walk to `parent == NULL` -- plus a test that `GOSUB`s past a `LABEL` and then - branches to it from the top level. Wants checking against deviation 32 first, because - "`LABEL` still executes and re-files itself" is deliberate and the re-filing is the part - worth keeping. +27. **Three of the four prescans report against the wrong line**, and work around it by + putting `Line %d:` in the message text -- so the message names the number twice and the + prefix is wrong. `akbasic_runtime_check_targets()` shows the fix. **Issue #10.** -27. **Three of the four prescans report against the wrong line.** - `akbasic_runtime_set_mode()`'s handler builds its BASIC error line from - `environment->lineno`, which after a load is wherever the loader stopped -- usually the - last line of the program. The label, `DATA` and `TYPE` scans therefore print `? 47 :` for - a fault on line 3, and work around it by putting `Line %d:` in the message text - (`src/structtype.c:342`), so the message names the number twice and the prefix is wrong. +28. **Two lines carrying the same written line number still silently keep the last.** The one + collision that can appear in an existing program, so it carries corpus risk the other three + did not. **Issue #11.** - `akbasic_runtime_check_targets()` does not have the problem because it points - `environment->lineno` at the line it is walking. That is the fix, and it belongs in the - wrapper rather than in four places: give each prescan a way to say which line it failed - on, or have each set the cursor as the target check does. Cosmetic, but it is the first - number a person reads when a program will not start. - -28. **Two lines carrying the same written line number still silently keep the last.** - `akbasic_runtime_file_line()` refuses a collision that involves an *assigned* number, but - leaves numbered-onto-numbered alone, which is what a file with `10 PRINT "A"` twice has - always done. A duplicate in a hand-written file is a mistake rather than an intent, and - refusing it would be consistent with the other three cases. - - Not changed with the rest because it is the one collision that can appear in an existing - program, so it carries corpus risk the others do not: 41 reference cases and 22 local ones - would all have to be shown clean first. `src/runtime.c`, in the `FAIL_NONZERO_RETURN` that - already names the slot. - -29. **`akbasic_renumber()` keeps two 9999-entry `static` locals.** - `src/renumber.c` holds `map[]` and `rewritten[]` at file scope, and - `akbasic_runtime_check_targets()` now adds a `static` scratch buffer beside them. That is - against the no-file-scope-mutable-state rule in `MAINTENANCE.md`, and it means `RENUMBER` - and the target prescan are not reentrant across two `akbasic_Runtime`s in one process -- - the exact thing that rule exists to guarantee. - - They are `static` because they will not fit on a default stack, which is the same reason - `akbasic_Runtime` itself is too big for one. The honest fix is to hang them off the - runtime like every other pool, which costs another ~2.5MB inline per interpreter for - something used by one verb and one prescan. Recorded rather than done, because "make it - reentrant" and "do not grow the runtime by a third for a scratch buffer" are both right - and picking between them is a decision. +29. **`akbasic_renumber()` keeps two 9999-entry `static` locals**, so `RENUMBER` and the target + prescan are not reentrant across two runtimes in one process. Hanging them off the runtime + costs ~2.5MB per interpreter for something one verb uses; both answers are defensible. + **Issue #12.** ### Mutation-testing gaps in the files the game work touched Recorded the way §8's CI note records `src/audio_tables.c`: a real test gap rather than a reason to avoid the file. -34. **`src/runtime.c` has never had a complete mutation run, and the one attempted here - was cut off at 551 of 997 mutants** after ninety minutes. What it did cover included - all three regions the value-pool, trap and skipped-block work touched, and the - survivors there are worth naming: - - - `report_and_reraise()`: **closed.** Deleting the `LOG_ERROR_WITH_MESSAGE` survived, - because nothing looked at the log -- and that line *is* the visible half of item - 33's second fix. `tests/trap_verbs.c` now redirects `akerr_log_method` into a buffer - and drives the path with a sink that refuses every write, which kills it. - - The skipped-block guard (`src/runtime.c`, the `BEND` test): four survivors, and - three of them are equivalent for any program that can be written -- `loopFirstLine - != 0` mutated to `!= 1` differs only for a loop on line 1, `waitingForCommand[0]` - to `[1]` only for a one-character wait, and flipping `strcmp(...) == 0` to `!= 0` - moves the release from the `FOR` to the `NEXT` that must follow it. A test with a - loop on line 1 would kill the first. The other two are noted rather than chased. - - A NULL-argument guard in `akbasic_runtime_reserve_globals()`, which is the same - survivor class every file in this tree has: nothing calls these with NULL. - - The remaining 446 mutants are unexamined. A full run is a release-workflow job, not a - per-commit one -- `.gitea/workflows/release.yaml` already mutates the whole tree -- and - what this entry is for is that the *partial* result has been read rather than filed - unopened. - - Measured alongside: `src/variable.c` scores 68.2% with **every mutant on the inline- - storage lines killed**, and `src/runtime_trap.c` 82.5% with **no survivors** in - `set_error_variables()`. Both files' remaining survivors are pre-existing and are - mostly NULL guards and `MAX - 1` arithmetic in string helpers. +34. **`src/runtime.c` has never had a complete mutation run** -- the one attempted was cut off + at 551 of 997 mutants. What it covered has been read rather than filed unopened, and three + of the four skipped-block survivors are equivalent mutants. **Issue #24.** ### Found while writing a game against the interpreter @@ -2187,110 +2030,18 @@ the same shape as item 12 above — a parser function that consumes one operator should loop — and the third is why both games are built out of `LABEL` and `GOTO`. 35. **An expression mixing `-` and `+` at the same level is evaluated right to left.** - `a - b + c` computes `a - (b + c)`. A C128, and every BASIC this descends from, folds a - run of same-precedence additive operators left to right. + `10 - 3 + 2` is 5 where a C128 gives 9, silently. The cause is the grammar's shape: two + additive precedence levels where there should be one. **Issue #1.** - ```basic - PRINT 10 - 3 + 2 - PRINT 1 - 2 - 3 - ``` +36. **Only one unparenthesised `AND` or `OR` is matched per expression**, so a three-term + condition is refused. `logicalandor()` is an `if` where `subtraction()` is a `while` -- the + same defect as item 12, on the operator pair item 12 did not reach. **Issue #2.** Worth + doing in the same sitting as item 35. - ```output - 5 - -4 - ``` - - The second line is right and the first is not; 9 is the answer. **Nothing fails** — this - is item 4's failure mode over again, a program that quietly computes something else — and - the two tutorials work around it by parenthesising every mixed `+` and `-` as a rule - rather than where it happens to matter. - - **The cause is the grammar's shape, not a missing loop.** `subtraction()` - (`src/parser.c:425`) sits *above* `addition()` (`:447`) as its own precedence level, and - both loop correctly — but `subtraction()`'s right operand is parsed by `addition()`, which - then consumes the `+` that should have been the *next* term of the subtraction. `1 - 2 - 3` - is right because the outer loop sees both minuses; `10 - 3 + 2` is wrong because the inner - one eats the plus first. - - The fix is one additive level rather than two: a single function matching `PLUS` and - `MINUS` in one `while`, building left-associatively, with `multiplication()` beneath it. - That is what item 12 left half-done — it made each of the two levels fold left without - noticing that there should only have been one. Touches `src/parser.c` and wants cases in - `tests/parser_expressions.c` beside item 12's, asserting `10 - 3 + 2` is 9 and - `10 + 3 - 2` is 11 (which already passes, and is the asymmetry that gives it away). - -36. **Only one unparenthesised `AND` or `OR` is matched per expression.** A second one is left - in the token stream, and in the place a program actually writes them — a condition — the - `THEN` check then finds the wrong token and the line is refused: - - ```basic - X# = 1 - Y# = 2 - IF X# = 1 AND Y# = 2 THEN PRINT "TWO IS FINE" - IF X# = 1 AND Y# = 2 AND X# = 1 THEN PRINT "THREE IS NOT" - ``` - - ```output - TWO IS FINE - ? 4 : PARSE ERROR Incomplete IF statement - ``` - - Parenthesising is the workaround and it works — `IF (A AND B) AND C THEN` parses — which - is what the tutorials tell a reader to do. It is a refusal rather than a wrong answer, so - it costs a puzzled minute rather than an evening; item 42 in §5 fixed the case of *one* - `AND` and stopped there. - - `logicalandor()` (`src/parser.c:329`) is `if ( akbasic_parser_match(obj, OPS, 2) )` at `:339` - where `subtraction()` at `:425` is `while`. **This is item 12's defect exactly**, on the - one operator pair item 12 did not reach, and the reference has the same shape at - `basicparser.go`. The fix is the same fix: `while`, folding left, reassigning `left` from - the leaf built on the previous pass. `tests/parser_expressions.c` for the expression form - and `tests/parser_commands.c` for the `IF` that made it visible. - - Worth doing together with item 35: they are one afternoon, one file, and between them they - are the two rules a tutorial currently has to spend a paragraph on. - -37. **A `GOTO` that leaves a `FOR` or a `DO` leaks the loop's scope**, so a program that uses - the pattern as its main loop stops after the thirty-second time round: - - ```basic - N# = 0 - LABEL TOP - DO - N# = N# + 1 - IF N# < 100 THEN GOTO TOP - LOOP UNTIL N# > 99 - PRINT "SURVIVED " + N# - ``` - - ```output - ? 3 : PARSE ERROR Environment pool exhausted at line 3 (32 in use) - ``` - - Thirty-two is `AKBASIC_MAX_ENVIRONMENTS` (`include/akbasic/types.h:31`), and the branch - that reports is the `DO` being entered for the thirty-third time rather than the `GOTO` - that did the damage — the same misattribution item 2 had. The `FOR` form behaves - identically. - - **This is a real pattern rather than a contrivance.** A game's main loop is a loop the - program leaves from the middle of: losing a life, clearing a level and quitting are all - "stop what you are doing and go somewhere else". Both Breakout listings in `examples/` - are built out of `LABEL`/`GOTO` for exactly this reason, which is the shape a reader of - chapters 17 and 18 is now told to copy, and it is a workaround rather than a preference — - a `DO ... LOOP` around the frame would be the natural way to write either one. - - The mechanism is the one item 2 documents from the other end: a loop's environment is - pushed when the line is parsed and popped by its `NEXT` or `LOOP`, and nothing else pops - it. `GOTO` sets the next line and returns (`akbasic_cmd_goto()`, `src/runtime_commands.c`); - it has no idea it has just left a scope. - - A fix has to know which scopes the branch target is *outside* of, which the environment - stack can answer if a loop's environment records the source line that pushed it: on a - `GOTO`, pop every loop scope whose line is not on the path to the target. That is more - machinery than items 35 and 36 want between them, and it is worth pricing against simply - documenting it — chapters 13 and 17 both now say "loop with `GOTO`, never jump out of a - `DO`", which is a rule a reader can follow. Wants `tests/for_next.c` either way, because - the reduction above is four lines and currently nothing asserts it in either direction. +37. **A `GOTO` that leaves a `FOR` or a `DO` leaks the loop's scope**, so a program using the + pattern as its main loop dies on the thirty-third pass -- and the error names the loop being + entered rather than the `GOTO` that did the damage. Both Breakout listings are built out of + `LABEL`/`GOTO` because of it. **Issue #3.** ### Found while integrating libakgl's collision subsystem @@ -2298,33 +2049,13 @@ should loop — and the third is why both games are built out of `LABEL` and `GO `akgl_CollisionShape` and a pooled proxy. Three things were deliberately left for later, and each is here so the reasoning does not have to be reconstructed. -38. **Finish moving `COLLISION` and `BUMP` onto the subsystem.** **Half done.** The contact - is no longer discarded: `RCOLLISION(n, f)` reports what was hit, the normal, the depth, - the contact point and the minimum translation axis, and - `tests/akgl_backends.c::test_contact_geometry` pins the normal's *sign* for both parties - of a sprite-against-sprite hit -- getting that backwards tells both to move the same way, - and nothing else would have noticed. What stands is the pairing and the threshold below. What landed is the narrowphase - and the shapes; the seam is still `collisions(self, uint16_t *mask)` and the pairing is - still all-against-all over eight slots with a bounding-box reject in front of it. - - That is the right shape *at eight sprites* and the benchmark says so: 54.9 ns a scan spread - out, 211.8 ns for breakout's layout, against a 1.20 ms frame. A uniform grid over eight - proxies costs more in per-proxy insert and move bookkeeping than an all-pairs loop over 28 - pairs saves, which is why the proxies are deliberately **not** registered with any - partitioner — `akgl_collision_test()` takes two positioned proxies and needs no world. - - **The threshold is what to record, not the work.** Registering the proxies and letting - `akgl_collision_query_box()` or the partitioner's `each_pair` do the pairing becomes worth - it somewhere above eight, and `AKBASIC_MAX_SPRITES` is a `#define` - (`include/akbasic/sprite.h:38`). Anybody raising it should have the number in front of them - rather than discovering the cost afterwards: re-run `tests/collision_perf.c` at the new - ceiling and compare against the frame row. Note that libakgl's own measurement puts the - naive all-pairs sweep at 0.7% of a frame at 64 actors and 11% at 256, so the crossover is - well above anything a Commodore-shaped sprite budget will reach. - - What remains is only the pairing seam above. The contact itself is consumed: - `akbasic_collision_service()` pulls it per sprite and the `RCOLLISION` fields are what both - breakout games now bounce on. +38. **Finish moving `COLLISION` and `BUMP` onto the subsystem.** **Half done** -- the contact + is no longer discarded and `RCOLLISION(n, f)` reports all of it, with + `tests/akgl_backends.c::test_contact_geometry` pinning the normal's *sign* for both parties. + What stands is the pairing seam, which is all-against-all over eight slots and is the right + shape at eight: 54.9 ns a scan against a 1.20 ms frame. **The threshold is the thing to + record, not the work** -- anybody raising `AKBASIC_MAX_SPRITES` should re-run + `tests/collision_perf.c` first. **Issue #14.** 39. ~~**Convert the breakout listings and chapters 17 and 18 to the collision verbs.**~~ **Done.** Both games and both tutorials now register their bricks with `SOLID`, arm @@ -2348,7 +2079,7 @@ each is here so the reasoning does not have to be reconstructed. ball that passes through the wall. Chapter 18 also lost a rule it depended on and no longer stated: a `FOR` with equal bounds - does not run its body (§6 item 19), which is why every list in the chapter has its one-item + does not run its body (§6 item 19, now issue #5), which is why every list in the chapter has its one-item case written out beside the loop. It is now stated in Step 7 where it is first used, rather than cross-referenced to a step that the rewrite deleted. @@ -2395,40 +2126,9 @@ each is here so the reasoning does not have to be reconstructed. candidates during the walk, close the walk, then dispatch. 41. **A multi-line `DEF` called outside a running program does not run its body**, and says - nothing about it. - - ```basic - DEF TRIPLE(N#) - T# = N# * 3 - RETURN T# - PRINT TRIPLE(14) - ``` - - At the REPL that prints `(UNDEFINED STRING REPRESENTATION FOR 0)`. In a program run from a - file the same function answers 42. No error either way. - - `akbasic_runtime_user_function()` (`src/runtime.c`) runs a multi-line body by handing - control to the call's environment and spinning a line loop until `RETURN` pops back out, - guarded on `mode == AKBASIC_MODE_RUN` -- which is true only of a program running from a - file. In `REPL` mode the loop is skipped entirely and the result is whatever is in the - caller's return slot, which is zero. The single-expression form has no such loop and is - unaffected, which is most of why this has gone unnoticed; and `tests/user_functions.c` - drives every case through `run_program`, so the whole suite is in RUN mode. - - **The obvious fix is wrong, and was tried.** Widening the guard to - `mode != AKBASIC_MODE_QUIT` makes the interpreter *hang* rather than answer wrongly: - `akbasic_runtime_process_line_run()` does not advance a REPL-mode runtime the way this - loop assumes, so the environment never comes back. Trading a silent wrong answer for a - lock-up is worse. The fix wants the REPL's own line cycle driving the body, which is a - change to how a call is executed rather than to a condition. - - It also bounds item 40: `akbasic_runtime_call_function()` reaches a multi-line body only - while a program is running, which is exactly the case a verb calling a callback is in and - not the case a host poking at a stopped runtime is in. - - A test belongs in `tests/user_functions.c` asserting the correct contract -- a multi-line - function answering 42 at the REPL -- registered in `AKBASIC_KNOWN_FAILING_TESTS` until it - does. + nothing about it -- the loop is guarded on `mode == AKBASIC_MODE_RUN`, and the whole suite + drives every case through `run_program`. **The obvious fix is wrong and was tried**: widening + the guard makes the interpreter hang rather than answer wrongly. **Issue #8.** 42. ~~**A name that merely contains a branch verb was treated as a branch.**~~ **Fixed.** The target prescan walks a line character by character looking for `GOTO`, `GOSUB`, @@ -2453,7 +2153,7 @@ each is here so the reasoning does not have to be reconstructed. ## 7. Filing gaps against `libakgl` When phase 7 or phase 8 needs something `libakgl` does not have, **stop and file it** in -`deps/libakgl/TODO.md` in that file's numbered prose style: what the BASIC verb requires, what +`libakgl`'s issue tracker: what the BASIC verb requires, what the `akgl_*` entry point should look like, and what tests would cover it. Growing `libakgl` to serve the interpreter is a wanted outcome, not a detour. @@ -2489,12 +2189,11 @@ covers libakgl's three widget helpers and nothing else. What a program cannot ha what closing each would cost: 1. **A top-centre or bottom-centre `HUD` anchor.** `akgl_UiAnchor` has the four corners - and dead centre, so `HUD` has exactly those five. Closing it upstream is two appended - enum values and two arms in `akgl_ui_label()`'s switch (`deps/libakgl/src/ui.c`), using - the `CLAY_ATTACH_POINT_CENTER_TOP` and `..._CENTER_BOTTOM` constants the dialog helper - in the same file already uses, with the horizontal inset dropped. Appending keeps 0 - through 4 meaning what they mean, so it is additive. **Not filed, and not done here: - changing a dependency submodule is that repository's decision, not this one's.** + and dead centre, so `HUD` has exactly those five. Two appended enum values and two arms in + `akgl_ui_label()`'s switch, using constants the dialog helper in the same file already uses. + **Filed upstream as `libakgl` #79** — this entry used to say "not filed, because changing a + dependency submodule is that repository's decision", which was the right instinct about + *changing* it and the wrong outcome for *reporting* it. 2. **Per-widget styles.** `UISTYLE` sets one look shared by everything, because `akgl_UiStyle` is per-call and threading an optional style number through three verbs' argument shapes buys less than it costs. Nothing upstream is missing. @@ -2502,7 +2201,8 @@ what closing each would cost: panels, custom elements, hit-testing. Reachable from C against `akgl/ui.h` in the same window; deliberately not reachable from BASIC. 4. **A dialog the player can dismiss.** libakgl has no dismiss contract — a declarative - frame is the visibility flag — so `DIALOG` with no argument is the only way down. + frame is the visibility flag — so `DIALOG` with no argument is the only way down. **Filed + upstream as `libakgl` #80.** One cost carried rather than fixed: libakgl re-rasterizes every line of widget text every frame, which is its own open `TODO.md` item. A `HUD` rebuilt every frame is the expensive @@ -2587,32 +2287,10 @@ None of the three was on any list. All were found by writing down what a chapter running it — which is the argument for the harness rather than a footnote to it. **A fourth is open, and it is the same defect as item 2 on a path that fix did not cover.** -Writing `docs/14-architecture.md` meant describing where the boundary between a script's -error and the host's error actually sits, and the answer is: around parsing and around -`interpret()`, but **not around scanning**. `akbasic_runtime_process_line_repl()` and -`akbasic_runtime_process_line_run()` both call `akbasic_scanner_scan()` under a bare `PASS` -(`src/runtime.c:825` and `:921`), so any error the scanner raises leaves `step()` as an -interpreter error. The reachable one is the token ceiling: - -```sh norun -$ printf 'PRINT 1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1\n' | ./build/basic -src/scanner.c:add_token:87: 515 (Out Of Bounds) : Line 0 has more than 32 tokens -... eight more frames ... -akbasic terminated on an unhandled error 515 (Out Of Bounds) -``` - -Exit status 1, a stack trace on stderr, and the prompt gone — for a line a person typed. An -embedding host is handed a context for what is plainly the script's mistake, which is what -goal 3 exists to prevent. It should print `? N : PARSE ERROR Line N has more than 32 tokens` -and stop the run, exactly as a parse error does two lines further down. - -The fix is the same `ATTEMPT`/`HANDLE_DEFAULT`/`akbasic_runtime_error()` wrapper already -sitting around `akbasic_parser_parse()` in both functions, moved up to cover the scan. -Three call sites to check, because `process_line_runstream()` has the same bare `PASS` and -its answer may want to differ: a bad line arriving from a file mid-load is not the same -situation as one typed at a prompt. Wants a test in `tests/scanner_tokens.c` asserting the -error line rather than the raised status, and one in `tests/housekeeping_verbs.c` asserting -the REPL survives it — the shape item 2's test already has. +The boundary between a script's error and the host's error sits around parsing and around +`interpret()`, but **not around scanning** — so the token ceiling takes the prompt down with a +stack trace for a line a person typed. **Issue #4**, which also names the third call site whose +answer may want to differ. **A separate one was found while designing optional line numbers, and is fixed.** `akbasic_runtime_process_line_runstream()` filed the *raw* buffer, line number and all, while @@ -2638,13 +2316,11 @@ and `src/graphics_tables.c` 100%, `src/runtime_input.c` 100%, `src/runtime_graph `src/runtime_audio.c` 98%, `src/play.c` 87%. The `akbasic_akgl` target is not in the coverage figure — it is not built in a default configuration. -**The denominator moved a long way and this is not an explanation of why.** The line total -went from 4076 to 6222 between the previous entry and this one, which is far more than the -work since added — so one of the two measurements is counting something the other is not, -and which one is right has not been established. Both figures were produced with -`gcovr --filter 'src/.*'` over a clean tree. Recorded rather than quietly overwritten, -because a coverage percentage whose denominator nobody can account for is worth exactly as -much as the accounting. The *ratio* is above the gate on either reading. +**The denominator moved a long way and this is not an explanation of why.** The line total went +from 4076 to 6222 between two entries, which is far more than the work added between them, so one +of the two measurements is counting something the other is not. Recorded rather than quietly +overwritten, because a coverage percentage whose denominator nobody can account for is worth +exactly as much as the accounting. The ratio is above the gate on either reading. **Issue #26.** **A note on reading that number, because it was misread once while producing it.** A stale `build-cov/` left in the source directory from an earlier session was silently folded into the @@ -2656,7 +2332,7 @@ and if a coverage number looks suspiciously unchanged, `find . -name '*.gcda'` b it. Branch coverage reads 18.0% and is not a target, for the reason `libakgl/TODO.md` and -`libakstdlib/TODO.md` both give: the akerror control-flow macros expand into large branch trees +`libakstdlib` both give: the akerror control-flow macros expand into large branch trees at every call site, most of them unreachable in normal operation. Track line and function coverage. @@ -2745,29 +2421,21 @@ What remains, in priority order: 2. ~~**A line editor for the akgl sink.**~~ **Done** — `readline` in `src/sink_akgl.c`, over the keystroke ring, borrowing frames from the host through an `akbasic_AkglPump`. It cannot type a shifted character, which is `libakgl` item 10 rather than work outstanding here. -3. **§4 — the language completion work queue**, and it is the only substantial thing left. - Done: groups G and I, the `GET`/`GETKEY`/`SCNCLR` part of E, group B, multiple statements - per line, and array references in parameter lists. Left: groups A, D, F and J, plus - `RESTORE` and `RENUMBER` out of B and the sprite group H. None needs anything from - `libakgl` except H. +3. **The language completion queue is done, and this entry used to say otherwise.** Every group + in §4's table — A through J, `RESTORE` and `RENUMBER` included — is implemented and tested. + The list that used to sit here naming A, D, F, J and H as outstanding was stale. - **Two pieces of structural work come before the verbs, and both were found rather than - guessed.** Neither is a verb and neither can be skipped by the group that needs it: + **What survives it is one piece of structural work, and it is not a verb.** Block skipping + works by source *line*, so a whole `FOR`/`NEXT` written on one line never reaches its `NEXT` + — inherited structure that did not matter until a line could hold more than one statement, + and `DO`/`LOOP`/`WHILE`/`UNTIL` are exactly the verbs people write on one line. **Issue #6.** - - **Block skipping works by source line, and group A needs it to work by statement.** - `waitingForCommand` skips forward to a verb one *line* at a time, so a whole loop written - on one line — `FOR I# = 1 TO 3 : PRINT I# : NEXT I#` — never reaches its `NEXT`. That is - inherited structure rather than a slip, and it did not matter until a line could hold more - than one statement. `DO`/`LOOP`/`WHILE`/`UNTIL` are exactly the verbs people write on one - line, so group A starts here or it ships something that does not work. - - **`READ` has no DATA pointer, and `RESTORE` is the verb that needs one.** §4 has the - detail; the short version is that `READ` skips forward to the next `DATA` *reached in - execution order*, so a `DATA` line placed before its `READ` is never found at all and a - second `READ` re-reads the same line. It is a live defect, not only a blocked verb. + `FILTER` is the one verb still refused, and deliberately: `akgl_audio_*` synthesises raw + waveforms and mixes them, there is no filter stage to configure, and SDL3 supplies no + primitive to build one from. It is refused with `AKBASIC_ERR_DEVICE` rather than silently + ignored — a program that asks for a low-pass and gets an unfiltered square wave has been + lied to. The libakgl half is that repository's issue #58. - Ordering suggestion, unchanged in spirit from the original: A (after the restructure), then - D and J, which are self-contained, then F, which is 21 verbs of `aksl_f*` and mostly - mechanical. H needs a look at `libakgl`'s sprite and actor API before anything is filed. 4. ~~**CI does not cover `-DAKBASIC_WITH_AKGL=ON`.**~~ **Done** — the `akgl_build` job in `.gitea/workflows/ci.yaml`. It turned out much cheaper than the deferral assumed, and the two reasons are worth keeping because both were guesses that measurement corrected: @@ -3037,56 +2705,17 @@ reduced against `build/basic`, the stdio build, unless it says otherwise. `tests/structure_verbs.c`, which today only exercises blocks whose bodies are plain statements. -3. **In the standalone SDL build, nothing the graphics verbs draw can be seen unless the - program shrinks the text area first.** **Half done.** The tee sink now forwards - `WINDOW` (§6 item 31), so `sink_window()` is reachable and a program *can* take rows - back from the text layer -- which is the half that made this unanswerable. What stands - is the default: the text area is the whole window, so a program that does not call - `WINDOW` still cannot see a drawing. +3. **In the standalone SDL build, the text layer owns every row of the window by default.** + **Half done.** The tee sink now forwards `WINDOW`, so a program *can* take rows back, and the + drawing verbs render into a layer the frame composites under the text and the sprites, so a + drawing survives the frame it was made in. - Whether that default is right is a real question rather than a defect, and it is - entangled with what no `WINDOW` call fixes -- the frontend never clears and SDL - double-buffers, so a drawing has to be re-issued every frame, and it has to fit inside - one batch to survive a capture (item 5). Both are now documented in chapters 6, 13 and - 14. **Sprites remain the only thing visible for free**, so the trick the game uses is - still the right one for a game; it should not be the *only* way, and it no longer is. - - The original report, whose second bullet is fixed and whose first stands: - - ```basic - GRAPHIC 1, 1 - COLOR 1, 3 - BOX 1, 100, 300, 700, 500 - PRINT "THE TEXT SHOWS AND THE BOX DOES NOT" - LABEL SPIN - GOTO SPIN - ``` - - The text appears; the box never does. Two mechanisms combine, and each alone would be - survivable: - - - `akbasic_sink_akgl_render()` fills **every row of the text area, opaque, every frame** - (`src/sink_akgl.c:564`), and the area is the whole window — `state->rows = h / cellh` at - `:505`. `akbasic_frontend_akgl_pump()` calls it after `akbasic_runtime_run()` and before - `SDL_RenderPresent`, so it paints over everything the program drew during those steps. - The comment at `:564` explains why it must repaint rather than track dirty rows, and that - reasoning is sound; what it does not account for is that the rows it owns are all of them. - - `WINDOW` would shrink the text area out of the way, and the akgl sink implements it - (`sink_window`, `src/sink_akgl.c:413`) — but **the tee sink in front of it never assigns - `obj->window`** (`src/sink_tee.c:143-151` assigns `clear` and conditionally `moveto`, and - stops). So `WINDOW` refuses with "needs a text device with a character grid" - (`src/runtime_console.c:179`) in the one build that has a character grid. - - The result is that **chapter 6 cannot be run in the interpreter it documents**. Its figures - are right because `tools/screenshot.c` deliberately omits the text layer — the comment in - that file says so — so the harness that proves the chapter is exactly the harness that hides - this. `docs_screenshots` passes and always would. - - Sprites are unaffected: `akbasic_sprite_akgl_render()` runs after the sink, which is why the - game draws its entire screen by `SSHAPE`-ing what it drew and installing it with `SPRSAV`. - That is a fine trick and it should not be the only way. - - The tee half is three lines and is worth doing whatever is decided about the rest. + What stands is the default, and it is a real question rather than a defect: a program that + does not call `WINDOW` still cannot see a drawing. **The consequence worth keeping is that + chapter 6 cannot be run in the interpreter it documents** — its figures are right because + `tools/screenshot.c` deliberately omits the text layer, so the harness that proves the chapter + is exactly the harness that hides this, and `docs_screenshots` passes and always would. + **Issue #23.** 4. ~~**Mixed-type arithmetic is decided by the left operand alone, and nothing says so.**~~ **Done, as documentation, by decision.** The behaviour is unchanged: a dialect is