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

Improving prefix-string parsing (fixes #2339) #2511

Merged
merged 6 commits into from
Feb 9, 2024

Conversation

mrdaybird
Copy link
Contributor

@mrdaybird mrdaybird commented Feb 8, 2024

Fixes #2339
Modifies how prefix-strings is parsed and tokenized.

  1. Tokenize prefixes ( 'r', 'b' etc) as Soft keyword KW_STR_PREFIX
  2. Update grammar to check for KW_STR_PREFIX followed by STRING

Now the compiler gives a syntax error when encountering a identifier(not a prefix) followed by a string literal.

@certik
Copy link
Contributor

certik commented Feb 8, 2024

Thanks! This PR looks good.

Can you write a test that failed (or gave a misleading error message) in master, and now works (gives a good error message)?

If it is just a better error message, you can put it into tests/error.

@certik certik force-pushed the fix-prefix-string branch from 83b4616 to daae9dc Compare February 9, 2024 01:15
@mrdaybird
Copy link
Contributor Author

Added the tests.
But to clarify, should I use the 'ast' flag or the 'ast_new' flag for the tests? are they even relevant for testing errors?

@certik
Copy link
Contributor

certik commented Feb 9, 2024

This looks great, thanks for fixing it and adding tests. I left some minor comments to fix.

But to clarify, should I use the 'ast' flag or the 'ast_new' flag for the tests? are they even relevant for testing errors?

Use ast to test syntax errors. Use asr to test semantic errors.

@certik certik marked this pull request as draft February 9, 2024 15:56
@certik
Copy link
Contributor

certik commented Feb 9, 2024

You have to update reference tests via ./run_tests -u.

@mrdaybird
Copy link
Contributor Author

Thanks I noticed a little late. 😅
Should I rebase as well?

@certik
Copy link
Contributor

certik commented Feb 9, 2024

Either polish the commits to a few logical commits and rebase, or just leave it and I'll squash and merge it.

@mrdaybird
Copy link
Contributor Author

mrdaybird commented Feb 9, 2024

I think you can just squash and merge it. I think I would probably mess things up with the rebase.

@mrdaybird mrdaybird marked this pull request as ready for review February 9, 2024 17:08
@certik certik merged commit 660aa2a into lcompilers:main Feb 9, 2024
13 checks passed
@mrdaybird mrdaybird deleted the fix-prefix-string branch February 10, 2024 07:56
Agent-Hellboy pushed a commit to Agent-Hellboy/lpython that referenced this pull request Mar 5, 2024
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.

old print syntax throws
2 participants