From d5f3c3ef5bdb8c3759d2604c5cdb479e54f43148 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sun, 2 Aug 2026 00:18:53 -0400 Subject: [PATCH] Accept GRAPHIC CLR and a negative DATA item Two parse handlers refusing something the documentation promises. Landing together because they are the same defect twice -- a verb's own argument shape falling through to a general path that cannot see it -- in one file, found by one program, and verified in one pass. **`GRAPHIC CLR`** is given as `GRAPHIC mode | CLR` in both docs/06-graphics.md and docs/11-verb-reference.md, and was refused: `CLR` is a verb of its own, so the generic arglist path scanned it as a command token and the expression parser answered "Expected expression or literal". `akbasic_parse_graphic()` takes it as this verb's keyword argument and emits mode 5 -- which `akbasic_cmd_graphic()` already treats as "drop the saved shapes and go back to text", so both spellings are one statement and the exec handler is untouched. The documentation was right all along; nothing in it changes. **`DATA -5`** was refused by `akbasic_parse_data()`, and only there: `READ` scans the source text directly (src/data.c) and always returned the -5 intact. So the value was right and *reaching* the statement raised -- which, since section 4 settled that `DATA` at run time is a no-op, is what a program does with every `DATA` line it walks past. A table of coordinates or velocities is full of negative numbers, which is how a game found it. The fix accepts a unary minus over a numeric literal and nothing else: `DATA -A#` is still a mistake worth naming, and `akbasic_leaf_is_literal()` keeps meaning what it says because other callers rely on it. tests/read_data.c covers both mechanisms -- reading a negative item and reaching the line after it -- with a mixed-sign table and a negative float, since the two were never the same code path. TODO.md section 9 items 7 and 8, struck. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 17 ++++++++++-- src/parser_commands.c | 60 +++++++++++++++++++++++++++++++++++++++++ src/verbs.c | 2 +- src/verbs.h | 1 + tests/parser_commands.c | 21 +++++++++++++++ tests/read_data.c | 32 ++++++++++++++++++++++ 6 files changed, 130 insertions(+), 3 deletions(-) diff --git a/TODO.md b/TODO.md index fac1c82..9215549 100644 --- a/TODO.md +++ b/TODO.md @@ -2751,7 +2751,13 @@ reduced against `build/basic`, the stdio build, unless it says otherwise. is not obvious from the failure — every brick simply comes out the colour of the last one captured. `tests/graphics_verbs.c` is where the regression goes. -7. **`GRAPHIC CLR` is documented and refused.** `docs/06-graphics.md:65` and +7. ~~**`GRAPHIC CLR` is documented and refused.**~~ **Done** -- `akbasic_parse_graphic()` + (`src/parser_commands.c`) takes `CLR` as this verb's keyword argument and emits mode 5, + which `akbasic_cmd_graphic()` already treats as exactly this, so both spellings are one + statement and the exec handler is unchanged. The documentation was right and the parser + was not; `tests/parser_commands.c` now pins the form the two chapters give. + + The original report: `docs/06-graphics.md:65` and `docs/11-verb-reference.md:54` both give the form as `GRAPHIC mode | CLR`. ```basic @@ -2770,7 +2776,14 @@ reduced against `build/basic`, the stdio build, unless it says otherwise. Worth noting because `docs_examples` cannot catch it: every fenced `GRAPHIC CLR` in the documentation is prose, not a tagged runnable block. -8. **A `DATA` line holding a negative number cannot be executed.** §4 settled that "`DATA` at +8. ~~**A `DATA` line holding a negative number cannot be executed.**~~ **Done** -- + `akbasic_parse_data()` accepts a unary minus over a numeric literal, which is the only + shape that was refused. Deliberately narrow: `DATA -A#` is still a mistake worth naming, + and `akbasic_leaf_is_literal()` keeps meaning what it says because it is used elsewhere. + `tests/read_data.c` covers reading one *and* walking past the line, which were two + different mechanisms and only one of them was ever wrong. + + The original report: §4 settled that "`DATA` at run time is now a no-op: it is a declaration, and reaching the statement means walking past it." Walking past a negative one raises instead: diff --git a/src/parser_commands.c b/src/parser_commands.c index 7cf2f05..082f143 100644 --- a/src/parser_commands.c +++ b/src/parser_commands.c @@ -57,6 +57,46 @@ akerr_ErrorContext *akbasic_parse_arglist(akbasic_Parser *parser, akbasic_ASTLea SUCCEED_RETURN(errctx); } +/* + * GRAPHIC mode [, clear] + * GRAPHIC CLR + * + * `CLR` is a keyword argument rather than an expression, and the generic arglist + * path cannot see it as one: `CLR` is a verb of its own, so it scans as a + * command token and the expression parser refuses with "Expected expression or + * literal". Both `docs/06-graphics.md` and `docs/11-verb-reference.md` have given + * the form as `GRAPHIC mode | CLR` all along, so the documentation was right and + * the parser was not. TODO.md section 9 item 7. + * + * It becomes mode 5, which is what akbasic_cmd_graphic() already treats as "drop + * the saved shapes and go back to text" -- so `GRAPHIC CLR` and `GRAPHIC 5` are + * the same statement, and the exec handler needs no change. + */ +akerr_ErrorContext *akbasic_parse_graphic(akbasic_Parser *parser, akbasic_ASTLeaf **dest) +{ + PREPARE_ERROR(errctx); + akbasic_ASTLeaf *arglist = NULL; + akbasic_ASTLeaf *expr = NULL; + akbasic_Token *peeked = NULL; + + peeked = akbasic_parser_peek(parser); + if ( peeked != NULL && peeked->tokentype == AKBASIC_TOK_COMMAND && + strcmp(peeked->lexeme, "CLR") == 0 ) { + (void)akbasic_parser_match1(parser, AKBASIC_TOK_COMMAND); + PASS(errctx, akbasic_parser_new_leaf(parser, &arglist)); + arglist->leaftype = AKBASIC_LEAF_ARGUMENTLIST; + arglist->operator_ = AKBASIC_TOK_FUNCTION_ARGUMENT; + PASS(errctx, akbasic_parser_new_leaf(parser, &arglist->right)); + PASS(errctx, akbasic_leaf_new_literal_int(arglist->right, "5")); + PASS(errctx, akbasic_parser_new_leaf(parser, &expr)); + PASS(errctx, akbasic_leaf_new_command(expr, "GRAPHIC", arglist)); + *dest = expr; + SUCCEED_RETURN(errctx); + } + PASS(errctx, akbasic_parse_arglist(parser, dest)); + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext *akbasic_parse_draw(akbasic_Parser *parser, akbasic_ASTLeaf **dest) { PREPARE_ERROR(errctx); @@ -942,6 +982,26 @@ akerr_ErrorContext *akbasic_parse_data(akbasic_Parser *parser, akbasic_ASTLeaf * FAIL_ZERO_RETURN(errctx, (arglist != NULL && arglist->right != NULL), AKBASIC_ERR_SYNTAX, "Expected literal"); for ( expr = arglist->right; expr != NULL; expr = expr->next ) { + /* + * A negated literal counts. `DATA -5` used to be refused here -- and only + * here: `READ` scans the source text directly (src/data.c) and returned + * the -5 intact, so the value was right and reaching the *statement* + * raised "Expected literal". Since section 4 settled that DATA at run + * time is a no-op, walking past one is exactly what a program does with + * it, and a table of coordinates or velocities is full of them. + * TODO.md section 9 item 8. + * + * Only over a numeric literal: `DATA -A#` is still a mistake worth + * naming, and akbasic_leaf_is_literal() keeps meaning what it says + * because it is used elsewhere. + */ + if ( expr->leaftype == AKBASIC_LEAF_UNARY && + expr->operator_ == AKBASIC_TOK_MINUS && + expr->left != NULL && + (expr->left->leaftype == AKBASIC_LEAF_LITERAL_INT || + expr->left->leaftype == AKBASIC_LEAF_LITERAL_FLOAT) ) { + continue; + } FAIL_ZERO_RETURN(errctx, akbasic_leaf_is_literal(expr), AKBASIC_ERR_SYNTAX, "Expected literal"); } diff --git a/src/verbs.c b/src/verbs.c index 6a3e4db..427487d 100644 --- a/src/verbs.c +++ b/src/verbs.c @@ -83,7 +83,7 @@ static const akbasic_Verb VERBS[] = { { "GETKEY", AKBASIC_TOK_COMMAND, -1, NULL, akbasic_cmd_getkey }, { "GOSUB", AKBASIC_TOK_COMMAND, -1, NULL, akbasic_cmd_gosub }, { "GOTO", AKBASIC_TOK_COMMAND, -1, NULL, akbasic_cmd_goto }, - { "GRAPHIC", AKBASIC_TOK_COMMAND, -1, akbasic_parse_arglist, akbasic_cmd_graphic }, + { "GRAPHIC", AKBASIC_TOK_COMMAND, -1, akbasic_parse_graphic, akbasic_cmd_graphic }, { "GSHAPE", AKBASIC_TOK_COMMAND, -1, akbasic_parse_arglist, akbasic_cmd_gshape }, { "HEADER", AKBASIC_TOK_COMMAND, -1, akbasic_parse_optional_arglist, akbasic_cmd_header }, { "HELP", AKBASIC_TOK_COMMAND_IMMEDIATE, -1, NULL, akbasic_cmd_help }, diff --git a/src/verbs.h b/src/verbs.h index 1782523..823e54a 100644 --- a/src/verbs.h +++ b/src/verbs.h @@ -17,6 +17,7 @@ /* Parse handlers -- src/parser_commands.c */ akerr_ErrorContext AKERR_NOIGNORE *akbasic_parse_arglist(struct akbasic_Parser *parser, akbasic_ASTLeaf **dest); akerr_ErrorContext AKERR_NOIGNORE *akbasic_parse_data(struct akbasic_Parser *parser, akbasic_ASTLeaf **dest); +akerr_ErrorContext AKERR_NOIGNORE *akbasic_parse_graphic(struct akbasic_Parser *parser, akbasic_ASTLeaf **dest); akerr_ErrorContext AKERR_NOIGNORE *akbasic_parse_draw(struct akbasic_Parser *parser, akbasic_ASTLeaf **dest); akerr_ErrorContext AKERR_NOIGNORE *akbasic_parse_def(struct akbasic_Parser *parser, akbasic_ASTLeaf **dest); akerr_ErrorContext AKERR_NOIGNORE *akbasic_parse_dim(struct akbasic_Parser *parser, akbasic_ASTLeaf **dest); diff --git a/tests/parser_commands.c b/tests/parser_commands.c index f52db1f..eb565dc 100644 --- a/tests/parser_commands.c +++ b/tests/parser_commands.c @@ -99,6 +99,27 @@ int main(void) expect_command("LABEL LOOPTOP", AKBASIC_LEAF_COMMAND, "LABEL"); expect_command("DIM A#(5)", AKBASIC_LEAF_COMMAND, "DIM"); expect_command("DATA 1, 2, 3", AKBASIC_LEAF_COMMAND, "DATA"); + /* + * A negated literal is one. `DATA -5` used to be refused here and only here: + * READ scans the source text directly and returned the -5 intact, so the + * value was right and reaching the *statement* raised "Expected literal" -- + * which is a table of coordinates or velocities failing on the line after + * it. TODO.md section 9 item 8. + */ + expect_command("DATA -5", AKBASIC_LEAF_COMMAND, "DATA"); + expect_command("DATA -1, 2, -3.5", AKBASIC_LEAF_COMMAND, "DATA"); + /* Only over a literal, though: a negated variable is still a mistake. */ + expect_parse_error("DATA -A#"); + + /* + * `GRAPHIC CLR` is documented in two chapters and used to be refused: CLR is + * a verb of its own, so the generic arglist path scanned it as a command + * token rather than as this verb's keyword argument. TODO.md section 9 item + * 7. It becomes mode 5, which the exec handler already treats as exactly + * this, so both spellings are one statement. + */ + expect_command("GRAPHIC CLR", AKBASIC_LEAF_COMMAND, "GRAPHIC"); + expect_command("GRAPHIC 1, 1", AKBASIC_LEAF_COMMAND, "GRAPHIC"); expect_command("READ A#, B#", AKBASIC_LEAF_COMMAND, "READ"); expect_command("POKE 100, 1", AKBASIC_LEAF_COMMAND, "POKE"); expect_command("INPUT \"NAME? \" A$", AKBASIC_LEAF_COMMAND, "INPUT"); diff --git a/tests/read_data.c b/tests/read_data.c index 7a76367..b861725 100644 --- a/tests/read_data.c +++ b/tests/read_data.c @@ -184,6 +184,37 @@ static void test_float_items(void) harness_stop(); } +/** + * @brief A `DATA` line holding a negative number can be read *and* walked past. + * + * Two different mechanisms, and only one of them was ever wrong. `READ` scans + * the source text directly, so it always returned the -5 intact; the statement's + * own argument parse refused a leading minus, so reaching the line raised + * "Expected literal". Section 4 settled that `DATA` at run time is a no-op -- + * reaching it means walking past it -- which is what made this reachable at all. + * + * TODO.md section 9 item 8. A table of coordinates or velocities is full of + * negative numbers, which is how a game found it. + */ +static void test_negative_items(void) +{ + TEST_REQUIRE_OK(run_program("10 READ A#\n" + "20 PRINT \"READ GAVE \" + A#\n" + "30 DATA -5\n" + "40 PRINT \"PAST IT\"\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "READ GAVE -5\nPAST IT\n"); + harness_stop(); + + /* Mixed signs across a real table, and a negative float. */ + TEST_REQUIRE_OK(run_program("10 DATA -1, 2, -3\n" + "20 DATA -0.5\n" + "30 READ A#, B#, C#\n" + "40 READ D%\n" + "50 PRINT \"\" + A# + \" \" + B# + \" \" + C# + \" \" + D%\n")); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "-1 2 -3 -0.500000\n"); + harness_stop(); +} + int main(void) { test_read_in_order(); @@ -197,5 +228,6 @@ int main(void) test_quoted_items(); test_colon_ends_data(); test_float_items(); + test_negative_items(); return akbasic_test_failures; }