1
0
Fork 0
mirror of https://github.com/imjasonh/nescript synced 2026-07-08 08:55:38 +00:00

codereview: address six findings from a fresh review pass

A focused review of the branch surfaced two correctness bugs and
four important polish items in the new features. None of the
existing example goldens shift — every fix is gated on conditions
that don't fire in the committed examples.

**debug.frame_overran() never reset between frames in implicit
wait_frame programs.** The IR-level WaitFrame op cleared $07FE,
but the implicit main-loop flag-clear that runs between dispatch
iterations only cleared ZP_FRAME_FLAG. A program whose
`on frame { ... }` body had no explicit `wait_frame` would latch
$07FE to 1 on the first miss and never reset, breaking
`debug.assert(not debug.frame_overran())` guards. The dispatch
loop now also clears $07FE in debug builds, mirroring the
WaitFrame path. New regression test asserts the main loop emits
exactly one STA $07FE in a no-wait_frame debug build.

**Metasprite base-tile resolution silently miscompiled for
`@chr` / `@binary` sprites.** The IR lowering walks
`program.sprites` to compute base tile indices but assumes
1 tile per non-Inline source, while the real asset resolver
reads the file. The analyzer now hard-rejects the combination
with a clear "use inline pixels" hint instead of letting it
compile to a visual glitch. New analyzer test
`analyze_metasprite_with_external_chr_sprite_errors` covers it.

**next_sprite_tile capping silently allowed CHR overlap.** The
pipeline used `.min(255)` which would let a background tile
overwrite a sprite tile when the sprite range filled the
pattern table. Now hard-errors via CompileError::AssetResolution
when the sprite range >= 256 *and* the program declares any
`@nametable(...)` background. Inline backgrounds aren't affected.

**Linker silently truncated background CHR overflow.** The
`if end <= chr.len()` guard at the CHR copy site dropped any
auto-CHR bytes that would have run past the pattern table.
Replaced with a debug assertion since the resolver should
have caught it upstream — defense in depth.

**Stale comment in nested_structs.ne** said struct literals
"don't accept array fields yet" while the example itself
demonstrates inline array fields working through
`expand_struct_literal_init`. Comment updated.

**Misleading sentinel comment in audio.rs** described the pitch
envelope's trailing zero as a runtime sentinel; in practice the
volume tick `JMP`s to `__audio_sfx_done` first and the pitch
update block never reads the trailing byte. Rewrote the comment
to clarify it's padding for predictable blob length.

Also tidies up two minor items the reviewer flagged:

- `flatten_struct_fields` rebuilt the `struct_sizes` HashMap on
  every leaf field; hoisted the snapshot to the function entry.
- Integration tests called `resolve_backgrounds(..., 0)` (the new
  `next_sprite_tile` parameter); changed to `1` so a future
  PNG-nametable test fixture won't accidentally overwrite the
  runtime smiley at tile 0.

https://claude.ai/code/session_01KEczoNUX3WmcFLfq6iAQxB
This commit is contained in:
Claude 2026-04-15 03:56:42 +00:00
parent 45bb84dddc
commit e8d602c1bc
No known key found for this signature in database
12 changed files with 195 additions and 34 deletions

View file

@ -28,9 +28,11 @@ struct Hero {
inv: u8[4],
}
// Hero positions are initialized inline so the on-frame handler
// only has to step them. Inventory bytes are a separate `var`
// because struct literals don't accept array fields yet.
// Hero positions are initialized inline. Both the nested
// `Vec2 { ... }` and the inline `inv: [1, 2, 3, 4]` array
// initializers are unpacked into per-leaf-field IR globals by
// `expand_struct_literal_init` in src/ir/lowering.rs, so each
// leaf gets its own `LDA #imm; STA addr` pair at reset.
var hero1: Hero = Hero { pos: Vec2 { x: 32, y: 96 }, hp: 100, inv: [1, 2, 3, 4] }
var hero2: Hero = Hero { pos: Vec2 { x: 200, y: 128 }, hp: 100, inv: [10, 20, 30, 40] }