Merge branch 'main' into 47-prepush

Reviewed-on: #50

.githooks/pre-push

@@ -0,0 +1,99 @@
+#!/usr/bin/env bash
+#
+# pre-push: run the cheap local gates before a commit reaches the forge.
+#
+# Install once per clone:
+#
+#     git config core.hooksPath .githooks
+#
+# The AKGL build and mutation harness are opt-in because they are expensive:
+#
+#     AKBASIC_HOOK_AKGL=1 git push
+#     AKBASIC_HOOK_MUTATION=1 git push
+#
+# Bypass everything with Git's escape hatch: git push --no-verify
+
+set -u
+
+ZERO_SHA=0000000000000000000000000000000000000000
+
+root=$(git rev-parse --show-toplevel) || exit 1
+cd "$root" || exit 1
+
+# A push that only deletes refs, or an empty push input, has no new commit to
+# test. Git calls hooks for those pushes too.
+has_updates=0
+while read -r _local_ref local_sha _remote_ref _remote_sha; do
+    if [ "$local_sha" != "$ZERO_SHA" ]; then
+        has_updates=1
+    fi
+done
+if [ "$has_updates" -eq 0 ]; then
+    exit 0
+fi
+
+if [ ! -f deps/libakerror/CMakeLists.txt ]; then
+    echo "pre-push: deps/libakerror is empty. Run:" >&2
+    echo "    git submodule update --init --recursive" >&2
+    exit 1
+fi
+
+builddir="${AKBASIC_HOOK_BUILD_DIR:-$(git rev-parse --git-dir)/akbasic-prepush}"
+mkdir -p "$builddir" || exit 1
+logfile="$builddir/last.log"
+
+# Keep output quiet on success, but retain the complete failing command output
+# so a failed push is actionable without rerunning the gate by hand.
+run() {
+    if ! "$@" >"$logfile" 2>&1; then
+        echo >&2
+        echo "pre-push: FAILED: $*" >&2
+        echo "---------------------------------------------------------------" >&2
+        cat "$logfile" >&2
+        echo "---------------------------------------------------------------" >&2
+        echo "pre-push: push aborted. Use 'git push --no-verify' to override." >&2
+        exit 1
+    fi
+}
+
+if [ -x scripts/cppcheck.sh ]; then
+    if command -v cppcheck > /dev/null 2>&1; then
+        echo "pre-push: cppcheck"
+        run scripts/cppcheck.sh
+    else
+        echo "pre-push: cppcheck not installed, skipping the optional check" >&2
+    fi
+else
+    echo "pre-push: scripts/cppcheck.sh not present, skipping the optional check" >&2
+fi
+
+echo "pre-push: default build + ctest"
+run cmake -S . -B "$builddir/default"
+run cmake --build "$builddir/default" --parallel 2
+run ctest --test-dir "$builddir/default" --output-on-failure
+
+echo "pre-push: sanitizer build + ctest"
+run cmake -S . -B "$builddir/sanitize" -DAKBASIC_SANITIZE=ON
+run cmake --build "$builddir/sanitize" --parallel 2
+run ctest --test-dir "$builddir/sanitize" --output-on-failure
+
+if [ "${AKBASIC_HOOK_AKGL:-0}" = "1" ]; then
+    echo "pre-push: AKGL build + ctest"
+    run cmake -S . -B "$builddir/akgl" -DAKBASIC_WITH_AKGL=ON
+    run cmake --build "$builddir/akgl" --parallel 2
+    run env SDL_VIDEODRIVER=dummy SDL_AUDIODRIVER=dummy \
+        SDL_RENDER_DRIVER=software ctest --test-dir "$builddir/akgl" --output-on-failure
+fi
+
+if [ "${AKBASIC_HOOK_MUTATION:-0}" = "1" ]; then
+    threshold="${AKBASIC_MUTATION_THRESHOLD:-65}"
+    mutants="${AKBASIC_HOOK_MUTANTS:-260}"
+    echo "pre-push: mutation testing, threshold ${threshold}%"
+    run python3 scripts/mutation_test.py \
+        --target src/symtab.c \
+        --max-mutants "$mutants" \
+        --threshold "$threshold"
+fi
+
+echo "pre-push: OK"
+exit 0

README.md

@@ -22,6 +22,18 @@ cmake --build build --parallel
 ctest --test-dir build --output-on-failure
 ```
 
+To run the same fast build, sanitizer and test gates before every push, install the advisory
+pre-push hook once in this clone:
+
+```sh norun
+git config core.hooksPath .githooks
+```
+
+The hook keeps its build trees under `.git/akbasic-prepush`, skips the optional cppcheck gate
+when its script or tool is unavailable, and leaves the AKGL build and mutation harness behind
+`AKBASIC_HOOK_AKGL=1` and `AKBASIC_HOOK_MUTATION=1`. `git push --no-verify` remains the escape
+hatch. `core.hooksPath` is local clone configuration, so CI remains the hard gate.
+
 ```sh norun
 ./build/basic                                          # the REPL
 ./build/basic tests/language/functions.bas             # run a program

docs/14-architecture.md

@@ -380,8 +380,10 @@ no free, so anything drawn from it is spent for the life of the run — and scop
 returns a variable's *slot* without returning its storage. A scalar therefore does not
 draw from it at all: `akbasic_variable_init()` points a one-element non-`@` variable at
 its own `inlinevalue`, which is what makes a local, a `FOR` counter and a `DEF` parameter
-free. Arrays and structures still spend, deliberately, because a pointer into a record is
+free. A pointer parameter is also free: it owns only its one-slot reference, so its
-allowed to outlive the scope that DIMmed it.
+`inlinevalue` dies with the call while the target remains in the caller's storage. Arrays,
+structures and by-value structure parameters still spend, deliberately, because a pointer
+into a record is allowed to outlive the scope that DIMmed it.
 [Chapter 13](13-differences.md) states the same budget from a BASIC programmer's side.
 
 The per-environment three are reset at the top of every line, which is what makes

docs/16-structures.md

@@ -266,6 +266,13 @@ The function saw 99; the caller still has 5. To change a caller's record on purp
 a pointer — `DEF POKEIT(P@ AS PTR TO CRATE)` — and reach through it with `->`. Neither is
 a special rule: both fall out of the parameter being assigned like any other variable.
 
+A pointer parameter's own reference is kept in the call variable's inline slot, so repeated
+calls do not spend the value pool. Its target remains the caller's structure. A by-value
+structure parameter is different: its copied slots remain in the value pool because
+`POINT Q@ AT B@` may retain a pointer to that copy after the function returns. Until escape
+analysis can distinguish that case, **do not call a by-value structure-parameter function in
+a loop**; bind a host global and rebind it per instance instead.
+
 ## What is checked, and what is not
 
 A field name is checked against the set the type declared, and the refusal lists the

include/akbasic/environment.h

@@ -277,6 +277,16 @@ akerr_ErrorContext AKERR_NOIGNORE *akbasic_environment_collect_subscripts(akbasi
  */
 akerr_ErrorContext AKERR_NOIGNORE *akbasic_environment_create(akbasic_Environment *obj, const char *varname, akbasic_Variable **dest);
 
+/**
+ * @brief Create a variable slot without allocating value storage.
+ *
+ * Used when the caller knows the variable's representation before its first
+ * initialization, such as a structure parameter. The caller must initialize
+ * the variable before evaluating it.
+ */
+akerr_ErrorContext AKERR_NOIGNORE *akbasic_environment_create_empty(akbasic_Environment *obj, const char *varname,
+										akbasic_Variable **dest);
+
 /**
  * @brief Resolve a label to the line number it marks.
  * @param obj Scope to search; the parent chain is walked.

include/akbasic/runtime.h

@@ -127,6 +127,12 @@ typedef struct akbasic_Runtime
 {
     akbasic_SourceLine source[AKBASIC_MAX_SOURCE_LINES];
 
+    /* Scratch owned by this runtime for RENUMBER and its target prescan. */
+    int16_t renumber_map[AKBASIC_MAX_SOURCE_LINES];
+    uint8_t renumber_visited[AKBASIC_MAX_SOURCE_LINES];
+    akbasic_SourceLine renumber_line;
+    char renumber_discard[AKBASIC_MAX_LINE_LENGTH * 2];
+
     /* Pools. Nothing here is malloc'd; everything is drawn from and returned. */
     akbasic_Environment environments[AKBASIC_MAX_ENVIRONMENTS];
     akbasic_Variable variables[AKBASIC_MAX_VARIABLES];

src/environment.c

@@ -310,7 +310,23 @@ akerr_ErrorContext *akbasic_environment_get(akbasic_Environment *obj, const char
     SUCCEED_RETURN(errctx);
 }
 
-akerr_ErrorContext *akbasic_environment_create(akbasic_Environment *obj, const char *varname, akbasic_Variable **dest)
+/**
+ * @brief Create a variable slot in the given scope, optionally allocating its storage.
+ *
+ * Shared by akbasic_environment_create() and akbasic_environment_create_empty(),
+ * which differ only in whether the new variable's value storage is initialized
+ * immediately or left for the caller to set up.
+ *
+ * @param obj Scope the variable is created in; only this scope is searched or
+ *        written to, unlike akbasic_environment_get()'s walk up the parent chain.
+ * @param varname Name of the variable to create.
+ * @param dest Set to the created (or already-existing) variable.
+ * @param initialize When true, the variable's value storage is allocated from
+ *        the runtime's value pool; when false, the caller must initialize it
+ *        before the variable is evaluated.
+ */
+static akerr_ErrorContext *environment_create_named(akbasic_Environment *obj, const char *varname,
+							akbasic_Variable **dest, bool initialize)
 {
     PREPARE_ERROR(errctx);
     akbasic_Variable *variable = NULL;
@@ -344,12 +360,49 @@ akerr_ErrorContext *akbasic_environment_create(akbasic_Environment *obj, const c
     PASS(errctx, aksl_strcpy(variable->name, sizeof(variable->name), varname));
     variable->valuetype = AKBASIC_TYPE_UNDEFINED;
     variable->mutable_ = true;
-    PASS(errctx, akbasic_variable_init(variable, &obj->runtime->valuepool, sizes, 1));
+    if ( initialize ) {
+	PASS(errctx, akbasic_variable_init(variable, &obj->runtime->valuepool, sizes, 1));
+    }
     PASS(errctx, akbasic_symtab_set(&obj->variables, varname, variable, 0));
     *dest = variable;
     SUCCEED_RETURN(errctx);
 }
 
+/**
+ * @brief Find a variable in this scope, creating and initializing it if absent.
+ *
+ * @param obj The scope to search and, on a miss, to create in.
+ * @param varname Name including its type suffix.
+ * @param dest Output destination populated with the variable.
+ * @return `NULL` on success, otherwise an error context owned by the caller.
+ */
+akerr_ErrorContext *akbasic_environment_create(akbasic_Environment *obj, const char *varname, akbasic_Variable **dest)
+{
+    PREPARE_ERROR(errctx);
+
+    PASS(errctx, environment_create_named(obj, varname, dest, true));
+    SUCCEED_RETURN(errctx);
+}
+
+/**
+ * @brief Find a variable in this scope, creating it without value storage if absent.
+ *
+ * The caller must initialize the variable before evaluating it.
+ *
+ * @param obj The scope to search and, on a miss, to create in.
+ * @param varname Name including its type suffix.
+ * @param dest Output destination populated with the variable.
+ * @return `NULL` on success, otherwise an error context owned by the caller.
+ */
+akerr_ErrorContext *akbasic_environment_create_empty(akbasic_Environment *obj, const char *varname,
+							akbasic_Variable **dest)
+{
+    PREPARE_ERROR(errctx);
+
+    PASS(errctx, environment_create_named(obj, varname, dest, false));
+    SUCCEED_RETURN(errctx);
+}
+
 /*
  * Evaluate an lvalue's subscript list, if it has one, into `subscripts`. A bare
  * identifier yields the single subscript {0}, which is how a scalar is addressed

src/renumber.c

@@ -72,7 +72,7 @@ struct akbasic_TargetWalk
  * rewritten: `GOTO 9999` in a program with no line 9999 is already broken, and
  * inventing a destination for it would hide that.
  */
-static int64_t mapped(const int64_t *map, int64_t line)
+static int64_t mapped(const int16_t *map, int64_t line)
 {
     if ( line < 0 || line >= AKBASIC_MAX_SOURCE_LINES ) {
 	return line;
@@ -306,7 +306,7 @@ static akerr_ErrorContext *rewrite_line(akbasic_TargetWalk *walk, const char *co
 static akerr_ErrorContext *visit_renumber(akbasic_TargetWalk *walk, int64_t target, char *dest, size_t len)
 {
     PREPARE_ERROR(errctx);
-    const int64_t *map = (const int64_t *)walk->self;
+    const int16_t *map = (const int16_t *)walk->self;
     int written = 0;
 
     PASS(errctx, aksl_snprintf(&written, dest, len, "%" PRId64, mapped(map, target)));
@@ -316,8 +316,7 @@ static akerr_ErrorContext *visit_renumber(akbasic_TargetWalk *walk, int64_t targ
 akerr_ErrorContext *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int64_t increment, int64_t oldstart)
 {
     PREPARE_ERROR(errctx);
-    static int64_t map[AKBASIC_MAX_SOURCE_LINES];
+    int16_t *map = obj == NULL ? NULL : obj->renumber_map;
-    static akbasic_SourceLine rewritten[AKBASIC_MAX_SOURCE_LINES];
     akbasic_TargetWalk walk = { map, visit_renumber };
     int64_t next = newstart;
     int64_t i = 0;
@@ -356,10 +355,8 @@ akerr_ErrorContext *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int
 	next += increment;
     }
 
-    PASS(errctx, aksl_memset(rewritten, 0, sizeof(rewritten)));
+    PASS(errctx, aksl_memset(obj->renumber_visited, 0, sizeof(obj->renumber_visited)));
     for ( i = 0; i < AKBASIC_MAX_SOURCE_LINES; i++ ) {
-	int64_t target = 0;
-
 	if ( obj->source[i].code[0] == '\0' ) {
 	    continue;
 	}
@@ -367,20 +364,77 @@ akerr_ErrorContext *akbasic_renumber(akbasic_Runtime *obj, int64_t newstart, int
 	 * Every line is rewritten, not just the moved ones: a line before
 	 * `oldstart` can branch into the region that moved.
 	 */
-	target = mapped(map, i);
 	PASS(errctx, rewrite_line(&walk, obj->source[i].code,
-				  rewritten[target].code, sizeof(rewritten[target].code)));
+				  obj->renumber_line.code, sizeof(obj->renumber_line.code)));
-	rewritten[target].lineno = target;
+	obj->renumber_line.lineno = i;
 	/*
 	 * Every line comes out numbered, whether or not it went in that way.
 	 * Asking for numbers is what RENUMBER is, and a program that has been
 	 * through it can be branched into by number -- which is the whole point
 	 * of running it over source that arrived without any.
 	 */
-	rewritten[target].numbered = true;
+	obj->renumber_line.numbered = true;
+	PASS(errctx, aksl_strncpy(obj->source[i].code, AKBASIC_MAX_LINE_LENGTH,
+				 obj->renumber_line.code, AKBASIC_MAX_LINE_LENGTH));
+	obj->source[i].lineno = obj->renumber_line.lineno;
+	obj->source[i].numbered = obj->renumber_line.numbered;
     }
 
-    PASS(errctx, aksl_memcpy(obj->source, rewritten, sizeof(obj->source)));
+    /* Move the already-rewritten lines in place. The map is a partial
+     * permutation: a chain ends at an empty slot, while a cycle closes back
+     * on its starting line. A single displaced line is sufficient for both. */
+    for ( i = 0; i < AKBASIC_MAX_SOURCE_LINES; i++ ) {
+	int64_t current = i;
+	akbasic_SourceLine displaced;
+	akbasic_SourceLine next_line;
+
+	if ( map[i] < 0 || obj->renumber_visited[i] ) {
+	    continue;
+	}
+	PASS(errctx, aksl_strncpy(displaced.code, AKBASIC_MAX_LINE_LENGTH,
+				 obj->source[i].code, AKBASIC_MAX_LINE_LENGTH));
+	displaced.lineno = obj->source[i].lineno;
+	displaced.numbered = obj->source[i].numbered;
+	for ( ;; ) {
+	    int64_t destination = map[current];
+
+	    obj->renumber_visited[current] = 1;
+	    if ( destination == i ) {
+		displaced.lineno = destination;
+		PASS(errctx, aksl_strncpy(obj->source[destination].code, AKBASIC_MAX_LINE_LENGTH,
+					 displaced.code, AKBASIC_MAX_LINE_LENGTH));
+		obj->source[destination].lineno = displaced.lineno;
+		obj->source[destination].numbered = displaced.numbered;
+		break;
+	    }
+	    if ( map[destination] < 0 ) {
+		displaced.lineno = destination;
+		PASS(errctx, aksl_strncpy(obj->source[destination].code, AKBASIC_MAX_LINE_LENGTH,
+					 displaced.code, AKBASIC_MAX_LINE_LENGTH));
+		obj->source[destination].lineno = displaced.lineno;
+		obj->source[destination].numbered = displaced.numbered;
+		PASS(errctx, aksl_memset(&obj->source[current], 0,
+					 sizeof(obj->source[current])));
+		break;
+	    }
+	    PASS(errctx, aksl_strncpy(next_line.code, AKBASIC_MAX_LINE_LENGTH,
+				     obj->source[destination].code, AKBASIC_MAX_LINE_LENGTH));
+	    next_line.lineno = obj->source[destination].lineno;
+	    next_line.numbered = obj->source[destination].numbered;
+	    displaced.lineno = destination;
+	    PASS(errctx, aksl_strncpy(obj->source[destination].code, AKBASIC_MAX_LINE_LENGTH,
+				     displaced.code, AKBASIC_MAX_LINE_LENGTH));
+	    obj->source[destination].lineno = displaced.lineno;
+	    obj->source[destination].numbered = displaced.numbered;
+	    PASS(errctx, aksl_memset(&obj->source[current], 0,
+				     sizeof(obj->source[current])));
+	    PASS(errctx, aksl_strncpy(displaced.code, AKBASIC_MAX_LINE_LENGTH,
+				     next_line.code, AKBASIC_MAX_LINE_LENGTH));
+	    displaced.lineno = next_line.lineno;
+	    displaced.numbered = next_line.numbered;
+	    current = destination;
+	}
+    }
     SUCCEED_RETURN(errctx);
 }
 
@@ -435,7 +489,6 @@ akerr_ErrorContext *akbasic_runtime_check_targets(akbasic_Runtime *obj)
      * step(). Nothing is read back out of it -- the walk needs somewhere to put
      * the text it would have written, and this is it.
      */
-    static char discard[AKBASIC_MAX_LINE_LENGTH * 2];
     CheckState state = { NULL, 0 };
     akbasic_TargetWalk walk = { &state, visit_check };
     int64_t entry = 0;
@@ -463,7 +516,8 @@ akerr_ErrorContext *akbasic_runtime_check_targets(akbasic_Runtime *obj)
 	 * the whole point of setting it.
 	 */
 	obj->environment->lineno = i;
-	PASS(errctx, rewrite_line(&walk, obj->source[i].code, discard, sizeof(discard)));
+	PASS(errctx, rewrite_line(&walk, obj->source[i].code,
+				 obj->renumber_discard, sizeof(obj->renumber_discard)));
     }
     /* Nothing was refused, so leave the cursor as the caller had it. */
     obj->environment->lineno = entry;

src/runtime.c

@@ -1040,12 +1040,13 @@ static akerr_ErrorContext *bind_structure_parameter(akbasic_Runtime *obj, akbasi
 		     obj->structtypes.types[typeindex].name,
 		     obj->structtypes.types[argvalue->structtype].name);
 
-    PASS(errctx, akbasic_environment_create(callenv, param->identifier, &variable));
+    PASS(errctx, akbasic_environment_create_empty(callenv, param->identifier, &variable));
     sizes[0] = (ispointer ? 1 : obj->structtypes.types[typeindex].slotcount);
+    /* A pointer parameter's own reference cannot escape its call scope. */
+    variable->ispointer = ispointer;
     PASS(errctx, akbasic_variable_init(variable, &obj->valuepool, sizes, 1));
     variable->valuetype = AKBASIC_TYPE_STRUCT;
     variable->structtype = typeindex;
-    variable->ispointer = ispointer;
 
     if ( ispointer ) {
 	PASS(errctx, akbasic_value_clone(argvalue, &variable->values[0]));

src/variable.c

@@ -99,18 +99,21 @@ akerr_ErrorContext *akbasic_variable_init(akbasic_Variable *obj, akbasic_ValuePo
      * 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
+     * **A `@` name is the one exclusion**, except for a one-slot pointer
-     * structure or a pointer to one keeps pool storage because a pointer may
+     * parameter. A structure or a pointer variable keeps pool storage because
-     * outlive the scope that DIMmed it -- docs/16-structures.md says nothing is
+     * a pointer may outlive the scope that DIMmed it -- docs/16-structures.md
-     * reclaimed and akbasic_runtime_prev_environment() relies on it. The suffix
+     * says nothing is reclaimed and akbasic_runtime_prev_environment() relies
-     * is the right test rather than `structtype`, which the DIM path sets
+     * on it. A pointer parameter owns only its reference slot; the target is
+     * owned by the caller, so bind_structure_parameter() marks it before this
+     * call and lets that slot use inline storage. The suffix is the right test
+     * for every other case 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 ( totalsize == 1 && lastchar != '@' ) {
+    if ( totalsize == 1 && (lastchar != '@' || obj->ispointer) ) {
 	obj->values = &obj->inlinevalue;
     } else if ( obj->values == NULL || obj->valuecount < (int)totalsize ) {
 	PASS(errctx, akbasic_valuepool_take(pool, (int)totalsize, &obj->values));

tests/user_functions.c

@@ -183,6 +183,33 @@ static void test_structure_parameters(void)
     harness_stop();
 }
 
+/**
+ * @brief Pointer parameters do not consume value-pool slots per call.
+ *
+ * The pointer's parameter variable owns only one reference to the caller's
+ * record, so that reference can live in the variable's inline slot. The target
+ * remains in the caller's storage. One pointer parameter and 8,000 calls make
+ * one leaked slot per call fail against the 4,096-slot value pool.
+ */
+static void test_pointer_parameters_do_not_leak_value_slots(void)
+{
+    TEST_REQUIRE_OK(run_program_bounded("10 TYPE CRATE\n"
+					"20   W#\n"
+					"30 END TYPE\n"
+					"40 DIM A@ AS CRATE\n"
+					"50 DIM P@ AS PTR TO CRATE\n"
+					"60 POINT P@ AT A@\n"
+					"70 DEF POKEIT(P@ AS PTR TO CRATE)\n"
+					"80 P@->W# = P@->W# + 1\n"
+					"90 RETURN P@->W#\n"
+					"100 FOR I# = 1 TO 8000\n"
+					"110 R# = POKEIT(P@)\n"
+					"120 NEXT I#\n"
+					"130 PRINT A@.W#\n", 1000000));
+    TEST_REQUIRE_STR(HARNESS_OUTPUT, "8000\n");
+    harness_stop();
+}
+
 /**
  * @brief A structure parameter must name its type, and the type is checked.
  *
@@ -454,6 +481,7 @@ int main(void)
     test_runaway_recursion_is_diagnosed();
     test_single_expression_form();
     test_structure_parameters();
+    test_pointer_parameters_do_not_leak_value_slots();
     test_structure_parameter_types_are_checked();
     test_call_scopes_are_reclaimed();
     test_calls_do_not_leak_value_slots();