Improve Corax+

[GamingMadster]

• Modified corax+
  • Fx33 Test now tests numbers that are <100.
  • 2NNN opcode now shows the X if not implemented.
  • If 00EE is not implemented, the test now handles for if the interpreter doesn't halt.
• Updated version number on all roms and images
  • The .html files that allow you to run the roms in Octo have also been updated.

[Timendus]

Hey there Madster!

Thanks for these additions and mad props for working your way through this kinda swampy code 😄. I have a couple of remarks below, and two overall points of feedback:

• Thanks for wanting to make this into a new version, images and all. However, for the discussion it's a lot easier to keep a PR small and easy to read. I'd rather have to do the release work myself, that way I can also decide if I want to put some more changes in 4.2, or if maybe this warrants a version 3. So please reduce this PR to just the changes in src.
• Mind your indentation; the repository is using two spaces where you are using one tab. It's good form and makes for easier to read diffs to conform to the indentation used by the project you're contributing too.

Thanks again, and I hope you find the time to work through this feedback. If not, I can pick it up and finish it, but I'd rather merge a commit by your hand and add your name to the list of contributors! 😄

[Timendus]

You can't have a begin without an end. Also, I usually stay away from putting multiple instructions on the same line, for readability. So this either has to be:

  if v0 != 1 begin
    i := image-no
    jump skipOtherFx33Tests
  end
  if v1 != 3 begin
    i := image-no
    jump skipOtherFx33Tests
  end
  if v2 != 7 begin
    i := image-no
    jump skipOtherFx33Tests
  end

Or, what's probably a bit nicer, and certainly smaller:

  i := image-no # This once, at the top

  # ... other code

  if v0 != 1 then jump skipOtherFx33Tests
  if v1 != 3 then jump skipOtherFx33Tests
  if v2 != 7 then jump skipOtherFx33Tests

  # ... other tests

  i := image-ok # this once, at the bottom, just before the label

[Timendus]

Labels go in column one (no indentation), except where we're doing self-modifying code magic or other strange things that aren't really labels in the traditional sense. Also, I would use a more descriptive name, like fx33-failed. Same goes for after0E above.

[Timendus]

Maybe I'm thinking about this too simply, but can't we just use flow-control for both tests? Do we need to keep this state?

[GamingMadster]

The idea for that was for if 2NNN were to fail, it could see that occur, and skip over 00EE properly. Sometimes, someone could have 00EE implemented, but not 2NNN, and in which case the interpreter would jump to $000. I wanted get the test to continue so the dev could see anything else that failed.

It could be considered unnecessary, but I'll leave that up to you to decide if I should remove it or not.

[Timendus]

No you're right... I've played with this a bit and it is indeed needed.

[Timendus]

I'd rather show nothing for 00EE instead of a cross when 2NNN isn't implemented, as technically it may work fine -- we just haven't tested it.

[GamingMadster]

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]

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

[Timendus]

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]

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]

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]

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!