Timendus / chip8-test-suite

A collection of ROM images with tests that will aid you in developing your own CHIP-8, SUPER-CHIP or XO-CHIP interpreter (or "emulator")
GNU General Public License v3.0
362 stars 7 forks source link

Improve Corax+ #22

Closed GamingMadster closed 2 months ago

GamingMadster commented 3 months ago
GamingMadster commented 2 months ago

Alright, I can't easily test all of the code right now as Windows struggles to build the test suite properly, and my Linux install is currently having a fit. However, the code should be fine from what I was able to test.

Please do make any more comments if necessary!

Timendus commented 2 months ago

Alright, I'll take a look. Do I have push access to your fork to make some changes to the PR?

Timendus commented 2 months ago

How about we just lean into the subroutine returning a value? Something like this:

: test2X
  v0 := 1
  return
  v0 := 2
  jump 2X-0E-hard-return

: main

  # ... other tests

  # Test calling and returning from subroutines (2NNN and 00EE)
  x0 := 18
  x1 := 22
  x2 := 27
  y := 1
  drawop im2 imX

  # Try to run subroutine
  v0 := 0
  test2X

: 2X-0E-hard-return
  i := image-ok
  if v0 == 0 then i := image-no  # Subroutine was never called
  sprite x2 y 4

  y += 5
  drawop im0 imE
  i := image-ok
  if v0 == 2 then i := image-no  # Return didn't work
  if v0 != 0 then sprite x2 y 4  # If subroutine wasn't called, return wasn't tested

I think it makes the code quite a bit simpler, right?

GamingMadster commented 2 months ago

How about we just lean into the subroutine returning a value? Something like this:

: test2X
  v0 := 1
  return
  v0 := 2
  jump 2X-0E-hard-return

: main

  # ... other tests

  # Test calling and returning from subroutines (2NNN and 00EE)
  x0 := 18
  x1 := 22
  x2 := 27
  y := 1
  drawop im2 imX

  # Try to run subroutine
  v0 := 0
  test2X

: 2X-0E-hard-return
  i := image-ok
  if v0 == 0 then i := image-no  # Subroutine was never called
  sprite x2 y 4

  y += 5
  drawop im0 imE
  i := image-ok
  if v0 == 2 then i := image-no  # Return didn't work
  if v0 != 0 then sprite x2 y 4  # If subroutine wasn't called, return wasn't tested

I think it makes the code quite a bit simpler, right?

Yeah, I'd agree on that. I'll push those changes to the fork really quick.

Timendus commented 2 months ago

Took some doing, but I've reduced this PR down to just the changes to the source, while keeping you as the author of the commit 😄 I'll give this a bit more testing later, have a meeting now. Let me know if you have any more comments!

GamingMadster commented 2 months ago

Honestly, all looks great on my end!

I might attempt some general code tidying-up later (likely will format the code to look a little nicer to the eye). Otherwise, this is pretty much ready to be merged.

It's an honor to have been able to contribute to this test suite!