Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix failing assertion with backslash at EOF in macro arguments #1634

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Jan 28, 2025

This was going to just increase test coverage, but it turns out that piping one of the test cases so it's read from stdin causes an assertion failure! 🙃 ...Which is manifesting as an infinite loop on some systems (Windows, Cygwin, and statically-linked macOS).

@Rangi42 Rangi42 added tests This affects the test suite rgbasm This affects RGBASM labels Jan 28, 2025
@Rangi42 Rangi42 added this to the 0.9.1 milestone Jan 28, 2025
@Rangi42 Rangi42 added the bug Unexpected behavior / crashes; to be fixed ASAP! label Jan 28, 2025
@Rangi42 Rangi42 changed the title Increase test case coverage Fix failing assertion with backslash at EOF in macro arguments Jan 28, 2025
@Rangi42
Copy link
Contributor Author

Rangi42 commented Jan 28, 2025

First, this case (reduced from macro-arg-escape-chars.asm):

MACRO mac
ENDM
; no newline, backslash is last char
mac \

was causing assume(size > 0); to fail in BufferedContent::advance().

Based on comparing to Expansion::advance(), I don't think the assumption was valid.

Fixing that (by doing if (size > 0) { size--; } instead of assuming it and always decrementing) then caused/uncovered this case (reduced from string-literal-macro-arg.asm):

MACRO mac
	println "\1"
	println \1
ENDM
; no newline, backslash is last char
mac "\

to print different output on Windows and MinGW than on all the other platforms.

I fixed that one in #1635 and rebased this to it.

`Expansion::advance()` can increase its offset beyond the size,
so I don't think this assumption was valid in the first place;
`BufferedContent::advance()` should be able to as well.
@Rangi42 Rangi42 force-pushed the more-tests branch 2 times, most recently from b11a208 to 3927ab5 Compare January 29, 2025 02:34
@Rangi42 Rangi42 merged commit c19ddc8 into gbdev:master Jan 29, 2025
26 checks passed
@Rangi42 Rangi42 deleted the more-tests branch January 29, 2025 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected behavior / crashes; to be fixed ASAP! rgbasm This affects RGBASM tests This affects the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant