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 file path and line number errors when using +, * and () #273

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

ydah
Copy link
Contributor

@ydah ydah commented Jul 20, 2024

This PR fix file path and line number errors when using +, * or ().

I discovered this when using the above new grammar in Lrama.
Refs: ruby/lrama@df39a6f
Refs: https://github.com/ruby/lrama/blob/df39a6f36535ac65e08a0a49eb84d6b944622e22/lib/lrama/parser.rb#L1270-L1282

As it stands, the second argument to module_eval is the absolute path to racc/grammarfileparser.rb and the line number.
Perhaps specifying @filename and @scanner.lineno + 1 is correct?

https://github.com/ydah/racc/blob/ea43e57012355c726b914c52fdcb32708d1091c6/lib/racc/grammarfileparser.rb#L373

@yui-knk
Copy link
Contributor

yui-knk commented Jul 28, 2024

Thanks for the PR. I think it's better to add test cases for this pattern. In this case, it's difficult to get line number and file name from actions, so I suggest to add test case in TestRaccCommand with assert_compile, assert_debugfile and assert_output_unchanged to check the output file difference. What do you think about it?

ydah added 2 commits July 30, 2024 02:08
This PR fix file path and line number errors when using `+`, `*` or `()`.
I discovered this when using the above new grammar in Lrama.
Refs: ruby/lrama@df39a6f
Refs: https://github.com/ruby/lrama/blob/df39a6f36535ac65e08a0a49eb84d6b944622e22/lib/lrama/parser.rb#L1270-L1282

As it stands, the second argument to `module_eval` is the absolute path to racc/grammarfileparser.rb and the line number. Perhaps specifying `@filename` and` @scanner.lineno + 1` is correct?

https://github.com/ydah/racc/blob/ea43e57012355c726b914c52fdcb32708d1091c6/lib/racc/grammarfileparser.rb#L373
@ydah
Copy link
Contributor Author

ydah commented Jul 29, 2024

@yui-knk Thank you for your reviewing! I add test case for TestRaccCommand. How about this?

@yui-knk yui-knk merged commit a71dd38 into ruby:master Jul 30, 2024
28 checks passed
@yui-knk
Copy link
Contributor

yui-knk commented Jul 30, 2024

Thank you!

@ydah ydah deleted the fix-filepath-lineno branch July 30, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants