diff --git a/CMakeLists.txt b/CMakeLists.txt index bbdbed7..c7aba75 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -305,6 +305,7 @@ set(AKBASIC_TESTS value_arithmetic value_bitwise value_compare + value_pool variable_subscript verbs_table version_check diff --git a/TODO.md b/TODO.md index bd9cc6b..94051c2 100644 --- a/TODO.md +++ b/TODO.md @@ -1863,8 +1863,22 @@ the reference's semantics reproduced faithfully; the third is the port's own. A complete Breakout, sprites and text grid and keyboard, running for minutes at a time. Nothing in either corpus runs for minutes, which is why the first of these had never been seen. -30. **A variable created inside a scope costs pool slots that never come back, so a long-running - program has a hard budget of 4096 name creations.** `akbasic_ValuePool` is a bump allocator +30. ~~**A variable created inside a scope costs pool slots that never come back, so a + long-running program has a hard budget of 4096 name creations.**~~ **Done** — a scalar now + lives in the variable rather than in the pool (`akbasic_Variable::inlinevalue`, + `src/variable.c`), so a `GOSUB` local, a `FOR` counter and a `DEF` parameter all cost + nothing at all. Twenty thousand scoped creations where four thousand used to be the whole + run. `tests/value_pool.c` is the coverage, and it asserts the mechanism as well as the + consequence — a later change that moved arrays inline too would pass every behavioural + case and break the pointer guarantee. **An array created in a scope still leaks**, which is + deliberate and is the same statement `akbasic_ValuePool` has always made: a pointer into a + record outlives the scope that DIMmed it. The record below is what was found and why. + + **Also fixed with it:** `SWAP` exchanges whole variable records, so the `values` pointer + that came over named the other variable's inline slot and SWAP silently did nothing. Caught + by `tests/language/housekeeping/verbs.bas`, which is the golden corpus earning its keep. + + The original report: `akbasic_ValuePool` is a bump allocator on purpose (`include/akbasic/value.h:30`), and the reason given is that "nothing in BASIC destroys a variable". **Scope exit destroys variables.** `akbasic_runtime_prev_environment()` (`src/runtime.c:121`) marks a popped environment's @@ -2440,8 +2454,11 @@ using the thing rather than by testing it.** The corpus reaches none of them. Ordered by blast radius. Each has a minimal reproduction that fits on a screen; every one was reduced against `build/basic`, the stdio build, unless it says otherwise. -1. **Every call to a user-defined function leaks value-pool slots, so a program may call one - about four thousand times and then die.** +1. ~~**Every call to a user-defined function leaks value-pool slots, so a program may call one + about four thousand times and then die.**~~ **Done** with §6 item 30, and by the same + change: the leaking slot was the call scope's parameter, which is a scalar and no longer + comes from the pool. Both `DEF` forms run eight thousand calls in + `tests/user_functions.c`. The original report: ```basic DEF ADDIT(N#) diff --git a/docs/17-tutorial-breakout.md b/docs/17-tutorial-breakout.md index 63731d3..cc680b4 100644 --- a/docs/17-tutorial-breakout.md +++ b/docs/17-tutorial-breakout.md @@ -91,65 +91,13 @@ A brick is four cells wide, so ten of them are forty cells, and `BLEFT#` centres whatever `COLS#` turned out to be. Integer division truncates here, which is exactly what a cell index wants. -## Step 3: Create every name before the game starts +## Step 3: Declare the names a subroutine has to answer through -This is the rule that decides whether your game runs for four minutes or for ever, and it -is worth getting straight before you write a single subroutine. +A `GOSUB` gets its own scope. Assignment walks up the chain and finds an outer name, but a +name **first seen** inside a subroutine is created in that subroutine and dies at +`RETURN` — so a routine cannot answer its caller through a name it invented itself. -**A name that is created costs value-pool slots that are never handed back.** The pool is -a bump allocator with 4096 slots for the life of the run. Scope exit *does* destroy -variables — a `GOSUB`'s locals go when it returns — but their slots are not reclaimed, so -recreating the same local on the next call takes fresh ones. - -Here is the whole defect on one screen: - -```basic -X# = 0 -FOR T# = 1 TO 6000 - GOSUB SUBA -NEXT T# -PRINT "OK " + X# -END - -LABEL SUBA -LOC# = 1 -X# = X# + LOC# -RETURN -``` - -```output -? 8 : RUNTIME ERROR Array of 1 elements does not fit in the 0 remaining value slots - -``` - -Six thousand calls, one new name each, and it dies on the four-thousand-and-somethingth -— naming the line that was unlucky rather than the line that was wrong. A game loop that -creates one name per tick is dead in half a minute. This one was, and it took twenty-five -seconds to do it. - -The fix is one line, moved: - -```basic -X# = 0 -LOC# = 0 -FOR T# = 1 TO 6000 - GOSUB SUBA -NEXT T# -PRINT "OK " + X# -END - -LABEL SUBA -LOC# = 1 -X# = X# + LOC# -RETURN -``` - -```output -OK 6000 -``` - -So **declare everything at the top** — variables, scratch temporaries, loop counters, all -of it — and after that the program only ever stores into names that already exist: +That is why the game declares this block before anything calls anything: ```basic norun DIM BR#(60) @@ -161,23 +109,66 @@ BX# = 0 BY# = 0 HIT# = 0 BI# = 0 -I# = 0 -R# = 0 -C# = 0 -KI# = 0 ``` -`I#`, `R#`, `C#` and `KI#` are in that list because a `FOR` counter is a name like any -other: name one that does not exist yet and every entry to the loop spends slots. +`HIT#` and `BI#` are how `HITTEST` (Step 8) answers the routine that called it. Declared +out here they are one variable the whole program shares; left to `HITTEST`, they would be +two that vanish at `RETURN` and the caller would read zeros for ever, with no diagnostic +of any kind. [Chapter 4](04-control-flow.md#goto-and-gosub) has the scope rules. -The same block does a second job. A `GOSUB` gets its own scope, and assignment walks up -the chain to find an outer name — but a name *first seen* inside a subroutine is created -in that subroutine and dies at `RETURN`. `HIT#` and `BI#` are how `HITTEST` answers its -caller, so they have to exist before anybody calls it. See -[Chapter 4](04-control-flow.md#goto-and-gosub) for the scope rules. +**Creating a scalar inside a scope is free**, so nothing else has to be declared up front: -This is filed as `TODO.md` §6 item 30. Until it is fixed, the declaration block is not a -style preference — it is the price of a program that runs. +```basic +X# = 0 +FOR T# = 1 TO 20000 + GOSUB SUBA +NEXT T# +PRINT "OK " + X# +END + +LABEL SUBA +LOC# = 1 +X# = X# + LOC# +RETURN +``` + +```output +OK 20000 +``` + +Twenty thousand calls, each creating a local. **This did not use to work.** A scalar drew +storage from the same 4096-slot pool an array does, scope exit gave back the variable but +not its storage, and a program creating one name per tick died in about half a minute — +naming whichever line was unlucky rather than the line that was wrong. It is what killed +the first version of this game after twenty-five seconds. `TODO.md` §6 item 30 has the +history; `tests/value_pool.c` is what keeps it fixed. + +**An array is different, and still costs slots for good.** `DIM` inside a subroutine takes +pool storage that never comes back, because a pointer into a record can outlive the scope +that declared it — see [Chapter 16](16-structures.md#limits): + +```basic +X# = 0 +FOR T# = 1 TO 6000 + GOSUB SUBA +NEXT T# +PRINT "OK " + X# +END + +LABEL SUBA +DIM LOC#(4) +LOC#(0) = 1 +X# = X# + LOC#(0) +RETURN +``` + +```output +? 8 : RUNTIME ERROR Array of 4 elements does not fit in the 0 remaining value slots + +``` + +So the rule that survives is narrow: **`DIM` at the top, not in a loop.** Which is where +you would have put it anyway. ## Step 4: Build the wall, and write a row whole @@ -717,8 +708,9 @@ every bounce, so it plays like something with a hand on it and occasionally miss demo that tracks perfectly never loses a ball and therefore never tests losing one. Leave it running for two minutes and watch the score climb: that exercises the ball, the -bricks, the level change, the speed-up and — most usefully — the value pool from Step 3, -which is the failure that only shows up after thousands of ticks. +bricks, the level change and the speed-up — and it is the only thing that exercises them +*together*, for long enough. Every defect this game found took thousands of ticks to show +itself, and a green test suite proved nothing about any of them. The machine does not get on the scoreboard. Attract mode keeps score so the wall behaves, but only a player's score is ever the high one: @@ -739,7 +731,7 @@ game against: | Rule | Because | Where | |---|---|---| -| Create every name at the top, loop counters included | A name created inside a scope costs pool slots for good; 4096 ends the run | Step 3 | +| `DIM` at the top, never in a loop | An array created inside a scope costs pool slots for good; 4096 ends the run. A scalar is free | Step 3 | | Build a text row whole and write it from column 0 | `CHAR` terminates the row where it stops | Step 4 | | Tell rows apart by shape, not colour | `CHAR` ignores its colour argument | Step 4 | | Compute a `MOVSPR` coordinate into a variable first | A leading sign means *move by*, not *move to* | Step 5 | diff --git a/docs/18-tutorial-breakout-artwork.md b/docs/18-tutorial-breakout-artwork.md index 3184176..7766a0d 100644 --- a/docs/18-tutorial-breakout-artwork.md +++ b/docs/18-tutorial-breakout-artwork.md @@ -773,7 +773,7 @@ stops a feature costing an afternoon before it is abandoned: | Variables | 128 | 121, plus 4 the interpreter makes | | Labels | 64 | 61 | | `SSHAPE` slots | 16, none reclaimed except by `GRAPHIC 5` | 6 stamps plus 1 capture a frame | -| Value-pool slots | 4096, none reclaimed | none after startup — see [Chapter 17](17-tutorial-breakout.md#step-3-create-every-name-before-the-game-starts) | +| Value-pool slots | 4096 for arrays and structures; a scalar costs none | six arrays, declared once — see [Chapter 17](17-tutorial-breakout.md#step-3-declare-the-names-a-subroutine-has-to-answer-through) | | Scopes | 32 | 6 deep at most | | Tokens on a line | 32, and the 33rd kills the interpreter rather than raising | short lines, temporaries instead of long conditions | diff --git a/include/akbasic/value.h b/include/akbasic/value.h index 4647a31..7426cc1 100644 --- a/include/akbasic/value.h +++ b/include/akbasic/value.h @@ -21,17 +21,24 @@ #include /* - * Backing store for array variables. Scalars and arrays alike draw their storage - * from here rather than from malloc, and a variable holds a pointer into it. The - * runtime owns exactly one; it is passed explicitly rather than kept at file - * scope, because the interpreter must be embeddable and may not own global - * mutable state. + * Backing store for arrays and structures. They draw their storage from here + * rather than from malloc, and a variable holds a pointer into it. The runtime + * owns exactly one; it is passed explicitly rather than kept at file scope, + * because the interpreter must be embeddable and may not own global mutable + * state. * - * The allocator is a bump: releasing is not supported, because nothing in BASIC - * destroys a variable. Re-DIMming a variable to the same size or smaller reuses - * its existing slice; growing it takes fresh slots and abandons the old ones. A - * program that re-DIMs the same array larger in a loop will exhaust the pool and - * get AKBASIC_ERR_BOUNDS -- bounded and diagnosable, which is the point. + * **A scalar is not in here.** It lives in the variable itself + * (akbasic_Variable::inlinevalue), because scope exit *does* destroy variables + * -- it returns the slot but not its storage -- and a bump allocator has no way + * to take a scalar back. See akbasic_variable_init(). + * + * The allocator is a bump: releasing is not supported. Re-DIMming an array to + * the same size or smaller reuses its existing slice; growing it takes fresh + * slots and abandons the old ones. A program that re-DIMs the same array larger + * in a loop will exhaust the pool and get AKBASIC_ERR_BOUNDS -- bounded and + * diagnosable, which is the point. A local *array* therefore still costs slots + * that do not come back, which is the same statement and is deliberate: a + * pointer into a record outlives the scope that DIMmed it. */ typedef struct { diff --git a/include/akbasic/variable.h b/include/akbasic/variable.h index bfc9dd8..1216585 100644 --- a/include/akbasic/variable.h +++ b/include/akbasic/variable.h @@ -20,8 +20,21 @@ typedef struct { char name[AKBASIC_MAX_STRING_LENGTH]; akbasic_Type valuetype; - akbasic_Value *values; /** Points into the runtime's value pool */ + akbasic_Value *values; /** The pool, or `inlinevalue` for a scalar */ int valuecount; + /* + * A scalar's storage, so that creating one costs the value pool nothing. + * + * The pool is a bump allocator with no free, and a scope exit hands the + * variable *slot* back while its storage stays counted against the pool -- + * so a name first created inside a GOSUB used to spend slots on every call + * and 4096 creations ended the run. Keeping the one value here instead + * means a local, a FOR counter and a DEF parameter are all free. + * + * akbasic_variable_init() decides which of the two `values` points at, and + * a `@` name is the one exclusion: see the comment there. + */ + akbasic_Value inlinevalue; int64_t dimensions[AKBASIC_MAX_ARRAY_DEPTH]; int dimensioncount; bool mutable_; diff --git a/src/runtime_housekeeping.c b/src/runtime_housekeeping.c index f66cddc..507ef4a 100644 --- a/src/runtime_housekeeping.c +++ b/src/runtime_housekeeping.c @@ -300,6 +300,18 @@ akerr_ErrorContext *akbasic_cmd_swap(akbasic_Runtime *obj, akbasic_ASTLeaf *expr memcpy(b, &swap, sizeof(*b)); memcpy(a->name, namea, sizeof(a->name)); memcpy(b->name, nameb, sizeof(b->name)); + /* + * A scalar's storage is *inside* the record, so the pointer that came over + * with it names the other variable's inline slot -- which now holds this + * variable's own old value. Left alone, each side reads back what it started + * with and SWAP silently does nothing. Rebind each to its own. + */ + if ( a->values == &b->inlinevalue ) { + a->values = &a->inlinevalue; + } + if ( b->values == &a->inlinevalue ) { + b->values = &b->inlinevalue; + } SUCCEED_TRUE(obj, dest); SUCCEED_RETURN(errctx); diff --git a/src/variable.c b/src/variable.c index 7473a05..38cc7e9 100644 --- a/src/variable.c +++ b/src/variable.c @@ -89,12 +89,30 @@ akerr_ErrorContext *akbasic_variable_init(akbasic_Variable *obj, akbasic_ValuePo obj->dimensioncount = sizecount; /* - * Reuse the existing slice when it is already big enough. That makes a - * re-DIM to the same or a smaller size free, which is the only re-DIM any - * real program performs; growing takes fresh slots and abandons the old - * ones, as documented on akbasic_ValuePool. + * A scalar lives in the variable; only a run of more than one value comes + * from the pool. + * + * That is not an optimisation, it is what makes a name created inside a + * scope cost nothing. The pool never frees and a scope exit returns the + * variable slot without returning its storage, so every GOSUB that created + * a local used to spend slots that never came back -- 4096 creations and + * the run was over, which a game loop reaches in half a minute. TODO.md + * section 6 item 30 has the whole reduction. + * + * **A `@` name is the one exclusion**, and it is the whole of it. A + * structure or a pointer to one keeps pool storage because a pointer may + * outlive the scope that DIMmed it -- docs/16-structures.md says nothing is + * reclaimed and akbasic_runtime_prev_environment() relies on it. The suffix + * is the right test rather than `structtype`, which the DIM path sets + * *after* calling this. + * + * Otherwise: reuse the existing slice when it is already big enough, which + * makes a re-DIM to the same or a smaller size free; growing takes fresh + * slots and abandons the old ones, as documented on akbasic_ValuePool. */ - if ( obj->values == NULL || obj->valuecount < (int)totalsize ) { + if ( totalsize == 1 && lastchar != '@' ) { + obj->values = &obj->inlinevalue; + } else if ( obj->values == NULL || obj->valuecount < (int)totalsize ) { PASS(errctx, akbasic_valuepool_take(pool, (int)totalsize, &obj->values)); } obj->valuecount = (int)totalsize; diff --git a/tests/user_functions.c b/tests/user_functions.c index f989572..d576914 100644 --- a/tests/user_functions.c +++ b/tests/user_functions.c @@ -22,15 +22,31 @@ #include "harness.h" #include "testutil.h" -/** @brief Run a program to completion, bounded so a hang fails rather than waits. */ -static akerr_ErrorContext AKERR_NOIGNORE *run_program(const char *source) +/** @brief Run a program to completion under an explicit step budget. */ +static akerr_ErrorContext AKERR_NOIGNORE *run_program_bounded(const char *source, int64_t steps) { PREPARE_ERROR(errctx); PASS(errctx, harness_start(NULL)); PASS(errctx, akbasic_runtime_load(&HARNESS_RUNTIME, source)); PASS(errctx, akbasic_runtime_start(&HARNESS_RUNTIME, AKBASIC_MODE_RUN)); - PASS(errctx, akbasic_runtime_run(&HARNESS_RUNTIME, 20000)); + PASS(errctx, akbasic_runtime_run(&HARNESS_RUNTIME, steps)); + SUCCEED_RETURN(errctx); +} + +/** + * @brief Run a program to completion, bounded so a hang fails rather than waits. + * + * 20,000 steps, which is generous for every case here except the two that call a + * function eight thousand times. Those name their own budget rather than raising + * this one, because a bound loose enough for them would stop the recursion case + * from failing quickly. + */ +static akerr_ErrorContext AKERR_NOIGNORE *run_program(const char *source) +{ + PREPARE_ERROR(errctx); + + PASS(errctx, run_program_bounded(source, 20000)); SUCCEED_RETURN(errctx); } @@ -232,6 +248,42 @@ static void test_call_scopes_are_reclaimed(void) harness_stop(); } +/** + * @brief Eight thousand calls, which used to be about four thousand too many. + * + * The *variable* slot came back (the test above); the value slots behind the + * parameter did not. A `DEF` cost one pool slot per call and died between the + * four and five thousandth with "Array of 1 elements does not fit in the 0 + * remaining value slots" -- so a game calling one function per frame at thirty + * frames a second got a little over two minutes, and the message named neither + * the function nor the reason. + * + * TODO.md section 9 item 1. Both forms are here because both leaked: the + * comparison that isolated it originally was that a `GOSUB` doing the same work + * ran eight thousand times without complaint. + */ +static void test_calls_do_not_leak_value_slots(void) +{ + TEST_REQUIRE_OK(run_program_bounded("10 DEF ADDIT(N#)\n" + "20 RETURN N# + 1\n" + "30 R# = 0\n" + "40 FOR I# = 1 TO 8000\n" + "50 R# = ADDIT(I#)\n" + "60 NEXT I#\n" + "70 PRINT \"OK \" + R#\n", 1000000)); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "OK 8001\n"); + harness_stop(); + + TEST_REQUIRE_OK(run_program_bounded("10 DEF SQR1(X#) = X# * X#\n" + "20 R# = 0\n" + "30 FOR I# = 1 TO 8000\n" + "40 R# = SQR1(I#)\n" + "50 NEXT I#\n" + "60 PRINT \"OK \" + R#\n", 1000000)); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "OK 64000000\n"); + harness_stop(); +} + int main(void) { TEST_REQUIRE_OK(akbasic_error_register()); @@ -244,6 +296,7 @@ int main(void) test_structure_parameters(); test_structure_parameter_types_are_checked(); test_call_scopes_are_reclaimed(); + test_calls_do_not_leak_value_slots(); return akbasic_test_failures; } diff --git a/tests/value_pool.c b/tests/value_pool.c new file mode 100644 index 0000000..51d7ed7 --- /dev/null +++ b/tests/value_pool.c @@ -0,0 +1,209 @@ +/** + * @file value_pool.c + * @brief Tests that creating a name inside a scope costs the value pool nothing. + * + * This is the defect a Breakout written against the interpreter died of after + * twenty-five seconds, and nothing in either corpus could have found it: the + * pool holds 4096 values, so it takes *thousands* of scope entries to see, and + * nothing else here runs that long. + * + * The cause was that scope exit returns the variable *slot* and not its storage. + * akbasic_runtime_prev_environment() marks the variable unused, + * akbasic_runtime_new_variable() memsets the slot it hands back -- clearing + * `values` -- and akbasic_variable_init() therefore drew fresh slots for a + * variable whose old ones were still counted. Every GOSUB that created a local + * leaked, with no diagnostic until the pool ran dry on whichever line happened + * to be unlucky. + * + * A scalar now lives in the variable itself, so the pool is not involved at all. + * Two of the counts below are past the old 4096 ceiling on purpose -- under it, + * they would pass against the broken interpreter too. The third takes the + * opposite approach and asks for the whole pool after only a few scopes, which + * is cheaper and sharper: one leaked slot and it has nowhere to go. + */ + +#include + +#include +#include + +#include "harness.h" +#include "testutil.h" + +/** @brief Run a program to completion, bounded so a hang fails rather than waits. */ +static akerr_ErrorContext AKERR_NOIGNORE *run_program(const char *source) +{ + PREPARE_ERROR(errctx); + + PASS(errctx, harness_start(NULL)); + PASS(errctx, akbasic_runtime_load(&HARNESS_RUNTIME, source)); + PASS(errctx, akbasic_runtime_start(&HARNESS_RUNTIME, AKBASIC_MODE_RUN)); + PASS(errctx, akbasic_runtime_run(&HARNESS_RUNTIME, 2000000)); + SUCCEED_RETURN(errctx); +} + +/** + * @brief 6,000 GOSUBs, each creating a local, cost nothing. + * + * TODO.md section 6 item 30's reduction, unchanged. It used to fail at + * `LOC# = 1` on the 4091st call with "Array of 1 elements does not fit in the + * 0 remaining value slots" -- naming the line that was unlucky rather than the + * line that was wrong, which is most of why it cost an evening to find. + */ +static void test_scoped_scalar_costs_nothing(void) +{ + TEST_REQUIRE_OK(run_program("10 X# = 0\n" + "20 FOR T# = 1 TO 6000\n" + "30 GOSUB SUBA\n" + "40 NEXT T#\n" + "50 PRINT \"OK \" + X#\n" + "60 END\n" + "70 LABEL SUBA\n" + "80 LOC# = 1\n" + "90 X# = X# + LOC#\n" + "100 RETURN\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "OK 6000\n"); + harness_stop(); +} + +/** + * @brief A `FOR` counter is a name like any other, and costs nothing either. + * + * Worth its own case: the counter is created by the loop rather than by an + * assignment the author wrote, so it was the half of this defect that a program + * could hit without appearing to create anything at all. + * + * `TO 2` rather than `TO 1` because equal bounds run the body zero times in this + * dialect -- see docs/13-differences.md. A body that never runs would pass this + * test for the wrong reason. + */ +static void test_scoped_loop_counter_costs_nothing(void) +{ + TEST_REQUIRE_OK(run_program("10 X# = 0\n" + "20 FOR T# = 1 TO 6000\n" + "30 GOSUB SUBA\n" + "40 NEXT T#\n" + "50 PRINT \"OK \" + X#\n" + "60 END\n" + "70 LABEL SUBA\n" + "80 FOR INNER# = 1 TO 2\n" + "90 X# = X# + 1\n" + "100 NEXT INNER#\n" + "110 RETURN\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "OK 12000\n"); + harness_stop(); +} + +/** + * @brief The pool is genuinely untouched, not merely large enough. + * + * Two hundred scope entries and then the pool's *entire* width -- four arrays, + * because AKBASIC_MAX_ARRAY_ELEMENTS caps any one of them at 1024 while the pool + * holds 4096. One leaked slot and the fourth `DIM` has nowhere to go, which + * makes this the sharpest of the three and by far the cheapest: the two above + * prove a program finishes, this proves nothing at all was spent. The long ones + * stay because they are the shape the defect was found in. + */ +static void test_pool_is_untouched_by_scopes(void) +{ + TEST_REQUIRE_OK(run_program("10 FOR T# = 1 TO 200\n" + "20 GOSUB SUBA\n" + "30 NEXT T#\n" + "40 DIM A#(1024)\n" + "50 DIM B#(1024)\n" + "60 DIM C#(1024)\n" + "70 DIM D#(1024)\n" + "80 D#(1023) = 7\n" + "90 PRINT D#(1023)\n" + "100 END\n" + "110 LABEL SUBA\n" + "120 LOC# = 1\n" + "130 RETURN\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "7\n"); + harness_stop(); +} + +/** + * @brief A structure keeps pool storage, which is what a pointer relies on. + * + * The exclusion that makes the fix safe. docs/16-structures.md says nothing is + * reclaimed and akbasic_runtime_prev_environment() says a pointer into a record + * DIMmed in a scope stays sound after the scope is gone -- so a `@` name must + * *not* move into the variable, where the next occupant of the slot would + * overwrite it. A single-field TYPE is the case that would slip through a test + * written against size alone. + */ +static void test_structure_storage_stays_in_the_pool(void) +{ + akbasic_Variable *variable = NULL; + + TEST_REQUIRE_OK(run_program("10 TYPE ONE\n" + "20 N#\n" + "30 END TYPE\n" + "40 DIM R@ AS ONE\n" + "50 R@.N# = 5\n" + "60 PRINT R@.N#\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "5\n"); + + TEST_REQUIRE_OK(akbasic_environment_get(HARNESS_RUNTIME.environment, "R@", &variable)); + TEST_REQUIRE(variable != NULL, "the structure variable must exist"); + TEST_REQUIRE(variable->values != &variable->inlinevalue, + "a structure must keep pool storage, not inline storage"); + harness_stop(); +} + +/** + * @brief A scalar's storage is inside the variable, and an array's is not. + * + * Asserted directly rather than only through a program, so the mechanism is + * pinned and not just its consequence. A future change that made arrays inline + * would pass every test above and break the pointer guarantee. + */ +static void test_storage_lands_where_it_should(void) +{ + akbasic_Variable *scalar = NULL; + akbasic_Variable *array = NULL; + + TEST_REQUIRE_OK(run_program("10 S# = 1\n" + "20 DIM A#(4)\n")); + + TEST_REQUIRE_OK(akbasic_environment_get(HARNESS_RUNTIME.environment, "S#", &scalar)); + TEST_REQUIRE(scalar != NULL, "the scalar must exist"); + TEST_REQUIRE(scalar->values == &scalar->inlinevalue, + "a scalar must be stored in the variable"); + + TEST_REQUIRE_OK(akbasic_environment_get(HARNESS_RUNTIME.environment, "A#", &array)); + TEST_REQUIRE(array != NULL, "the array must exist"); + TEST_REQUIRE(array->values != &array->inlinevalue, + "an array must be stored in the pool"); + harness_stop(); +} + +/** + * @brief `SWAP` exchanges two scalars, whose storage moves with the record. + * + * A regression rather than a feature: SWAP copies the whole variable record, so + * the `values` pointer that comes over names the *other* variable's inline slot + * -- which by then holds this variable's own old value. Left alone each side + * reads back what it started with and SWAP silently does nothing, which is + * exactly what the housekeeping golden case caught. + */ +static void test_swap_still_exchanges_scalars(void) +{ + TEST_REQUIRE_OK(run_program("10 A# = 1 : B# = 2\n" + "20 SWAP A#, B#\n" + "30 PRINT A# : PRINT B#\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "2\n1\n"); + harness_stop(); +} + +int main(void) +{ + test_scoped_scalar_costs_nothing(); + test_scoped_loop_counter_costs_nothing(); + test_pool_is_untouched_by_scopes(); + test_structure_storage_stays_in_the_pool(); + test_storage_lands_where_it_should(); + test_swap_still_exchanges_scalars(); + return akbasic_test_failures; +}