Skip to content

Accept C files ending in a backslash-newline #187

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

Merged
merged 5 commits into from
Apr 23, 2025

Conversation

Pennycook
Copy link
Contributor

Related issues

Proposed changes

  • Add a simple regression test for files that end in \, inspired by a header found in a Llama.cpp test.
  • Add the file being parsed to the debug log output. I added this to help me debug the test failure, and it seemed useful.
  • Remove "relaxed" parsing mode and improve the parsing error message.
  • Allow parsing to succeed when the last line is recognized as a continuation (i.e., it ends in \).

Enabling "relaxed" parsing mode would also have allowed the failing test to pass, but permitting any invalid code to proceed to future preprocessing phases seems like it would lead to errors that were almost impossible to debug. By providing an allowlist of permitted relaxed cases instead, we can generate a specific warning for each case and introduce additional relaxations over time.

I'll acknowledge that the way I've added a workaround for this specific case is a bit of a hack. A solution that detected \ at the end of a file and generated a filename:line:col: style warning immediately would be much better, but file_source isn't set up to track tokens that way.

Note that while removing the relaxed parameter is a breaking change to these interfaces, directly calling the internals of the codebasin package was deprecated in 1.2.0, so I don't think it's an issue.

gcc accepts this with a warning and clang accepts it silently, but CBI exits
with a RuntimeError.

Signed-off-by: John Pennycook <john.pennycook@intel.com>
Signed-off-by: John Pennycook <john.pennycook@intel.com>
This parsing mode has never been enabled, and referring to it when parsing
fails does not help end users.

Instead, we should:
- Treat any parsing failure as a bug; and
- Selectively permit certain failures (with warnings).

Signed-off-by: John Pennycook <john.pennycook@intel.com>
Signed-off-by: John Pennycook <john.pennycook@intel.com>
@Pennycook Pennycook added the bug Something isn't working label Apr 23, 2025
@Pennycook Pennycook added this to the 2.0.0 milestone Apr 23, 2025
@Pennycook Pennycook requested a review from laserkelvin April 23, 2025 09:17
Copy link
Contributor

@laserkelvin laserkelvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - just made the one suggestion on issue creation link

Ending the warning with a "." might result in a 404, if the terminal decides to
treat that as part of the URL.

Signed-off-by: John Pennycook <john.pennycook@intel.com>
@Pennycook Pennycook merged commit 6bcfbbc into intel:main Apr 23, 2025
4 checks passed
@Pennycook Pennycook deleted the bugfix/backslash-eof branch April 23, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants