bbbradsmith / nsfplay

Nintendo NES sound file NSF music player
https://bbbradsmith.github.io/nsfplay/
280 stars 45 forks source link

GCC/Clang-compatible token-pasting and silence gcc warning re: parentheses #19

Closed jprjr closed 4 years ago

jprjr commented 4 years ago

Some more small fixes - GCC/Clang had an error on pasting tokens that begin with a parenthesis, turns out you don't need to use the paste operator if the next character is a parenthesis.

Also silences a warning from GCC about surrounding an operand with parenthesis to make it less ambiguous.

bbbradsmith commented 4 years ago

Looks innocuous but because of the specific code it touches, I can't merge it until I can run a comprehensive CPU emulation test on it. Unfortunately, there's no automated test and I don't have the test NSFs prepared anymore (it's been years since the CPU code has really been touched).

So, no issue with it in theory, but it might take a little while before testing it gets onto my plate.

jprjr commented 4 years ago

I just did a quick test, it looks like the token-pasting commit produces byte-for-byte identical output, the warning-silencing commit does not. I'll open a separate PR for the token-pasting commit, that warning-silence commit will need testing.

Here's what I did to verify that output was the same:

Here's my results:

md5sums on master:

md5sums on token-pasting commit:

md5sums on warning-silence commit:

(nsfplug.exe and nezplug_ui.dll all had the same md5sum, as expected)

bbbradsmith commented 4 years ago

Thanks for investigating what changes.

The RTO8 and RTO16 changes might be significant, the look like they might fix bugs in the implementation of a few of the obscure illegal opcodes. (I feel like previous tests should have caught them, but due to the nature of these opcodes maybe they slipped through the cracks. Gives me something to look at later though.)

I will need to create a full CPU test NSF again for version 3 anyway, so at least that will be two birds with one stone. Since I'll have to do that before 2.5 to verify this potential bug, am comfortable just taking this pull now.

bbbradsmith commented 4 years ago

Actually, looking at it again, the ones I was suspicious about are entirely dependent on the & operator, so they might switch the order of & but shouldn't change the result... they're probably fine, but definitely dodging a bullet on that non-sanitized macro.