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

Add lineno #7

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

Add lineno #7

wants to merge 3 commits into from

Conversation

mingodad
Copy link
Contributor

@mingodad mingodad commented May 9, 2022

Add line number and column to error messages

@ChrisHixon
Copy link
Owner

I like the idea of adding line number tracking, but I don't think this pull request does this in a complete/correct way.

@mingodad
Copy link
Contributor Author

mingodad commented May 9, 2022

It's a start point that at least for the few cases I'm trying it now is showing a useful result, if we can nail down the flaws that you are seeing I also would like to have the file name on the error message, my editor can jump to the error line, column when the message start with the filename:

<filename>:line:column message

@ChrisHixon
Copy link
Owner

I've only looked at the diff here, but I think it misses some cases where a newline can occur, like inside a LIT. Perhaps more critical , offset is reset from the stack during backtracking, so I think the line:col tracking would need to be reset as well when backtracking (so saved on the stack). I think line:col tracking would have to follow offset through all changes to it or there will be cases where the tracking is incorrect. I dont think I like the idea of putting all this kind of line:col tracking inside the parser VM anyway. A better and simpler approach might be to find the error's line and col from the error_offset when an error is printed; just count the number of newlines that occur before error_offset for line number, and calculate the col looking back to the nearest newline position.

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.

2 participants