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 #444 by removing old parsing. #504

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

sampottinger
Copy link
Collaborator

@sampottinger sampottinger commented Jun 21, 2022

It looks like there's still some left over non-ANTLR parsing to check for missing braces from before the new preproc. The new preproc will catch the error and testing makes it look like this old regex check is not triggering anyway outside of comments (which is an issue reported in #444). I may be missing something here though @benfry... LMK what you think. Thanks!

It looks like there's still some left over regex parsing to check for missing braces from before the new preproc. The new preproc will catch the error and testing makes it look like this old regex check is not triggering anyway outside of comments (which is an issue reported in #444).
@sampottinger sampottinger requested a review from benfry June 21, 2022 15:52
@benfry
Copy link
Owner

benfry commented Jul 12, 2022

IIRC this is in place because 1) it can produce exceptionally confusing errors (especially for beginners), and 2) many moons ago, it could put the compiler into a bad state and cause a crash.

I assume the latter is resolved these days, but are we in good shape for the former?

@sampottinger sampottinger changed the title Fix #444 by removing old regex parsing. Fix #444 by removing old parsing. Jul 16, 2022
@sampottinger
Copy link
Collaborator Author

Thanks @benfry! For sure, the later is fixed. For the former... I'm not quite sure how it would get triggered because, in the case of a missing { or }, it would fail and finish execution in the preprocessing stage before it hits the old parsing logic anyway. So, right now, it can only be activated if the code is valid (it can only report an error if an error doesn't exist). Two options:

  • Maybe we could modify this to only check for the curly in the case of preproc failure? That being said, the logic does fail for the comments so there's still a chance that the old SourceUtils check could highlight "correct" code and miss the actual error.
  • We find a way for the preproc just to provide a more beginner friendly error message without SourceUtils.

What do you think? I might be more inclined towards the second option just to avoid other failure cases for the SourceUtils.

@benfry benfry merged commit dbb34bb into master Jul 20, 2022
@benfry benfry deleted the clean_remove_missing_curly branch July 20, 2022 00:14
@benfry
Copy link
Owner

benfry commented Jul 20, 2022

Ok, let's give it a shot for the next beta. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants