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

Uniform support for \n, \r\n, \r as line breaks. Fixes #4882. #4895

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jordanbrown0
Copy link
Contributor

Fixes lexer to count lines properly in files with \n, \r\n, and \r line breaks. (\r line breaks are used only in some old or obscure systems, but what the heck.)

Tweaks existing line number counting for (bad but legacy) include/use cases:

include <
line2>

reports a warning. Old behavior was to report it on line 2, but really the error (the unexpected end-of-line) is on line 1 so the new behavior (and tweak to existing test) makes it report line 1.

Adds a new RETVAL argument to add_cmdline_test() to allow tests where execution is expected to fail to still get expected-output comparison, to check that the right errors (and line numbers!) are reported. We should probably eventually move the existing add_failing_test cases to use this mechanism instead, so that they can check the error reported.

Adds a gitattributes file that suppresses text processing for the new *.bin.scad test files. Those files have sections that are UNIX-style, sections that are Windows-style, and an obscure \r-only section. In theory I think git will preserve them, but they may be too fragile for long-term use. We'll see.

@jordanbrown0
Copy link
Contributor Author

OBTW, I cloned a copy to a new directory, and the *.bin.scad files seemed to survive.

@jordanbrown0
Copy link
Contributor Author

Sigh. Looks like I fooled myself into thinking it fixed the problem, and need to fire up the Linux VM and check it out there.

@jordanbrown0
Copy link
Contributor Author

I found the (stupid) mistake, and fixed it.

One of the tests currently doesn't pass because it compares output, and the output includes a fully qualified path to the test .scad file, and of course those paths are different for different build environments.

@t-paul
Copy link
Member

t-paul commented Dec 28, 2023

Looking at this test fail, I wonder if we could just remove that log output. There's an error log from the parser yyerror before already.

@jordanbrown0
Copy link
Contributor Author

I’m fine with removing that log message too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants