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 user argument to some macros, moved from ruby/ruby to ruby/lrama #40

Merged
merged 2 commits into from
Jun 11, 2023

Conversation

alitaso345
Copy link
Contributor

Background

  • A change to modify the template was introduced in Remove ytab.sed hack ruby#7807 at the ruby/ruby side.
  • Initially, this case was not anticipated, resulting in a difference in the templates between ruby/ruby and ruby/lrama. As a result, we temporarily commented out the test that copies the template and compares the output.

Changes Made

  • Incorporated the template changes that were implemented on the ruby/ruby side. The changes we adopted are identical to those in ruby/ruby@8c3d5b0.
  • With this change, the part that was commented out was reinstated because copying the template will now work with ruby/ruby.
  • There was a test to confirm that the difference with binson was 0, but due to this change, they no longer match, and we considered its role to be completed and thus, removed the spec itself.

(日本語)

背景

  • Remove ytab.sed hack ruby#7807 にてruby/ruby側でテンプレートを変更する内容が入った。
  • 当初そのケースは想定しておらず、結果としてruby/ruby側とruby/lrama側でテンプレートに差分が出る形になった。そのためテンプレートをコピーし出力内容を比較するテストを一時的にコメントアウトしていた。

やったこと

  • ruby/ruby側で実装されていたテンプレートの変更を取り込んだ。取り込んだ内容は ruby/ruby@8c3d5b0 と全く同じもの。
  • 本変更により、テンプレートをコピーしてもruby/ruby側が動くようになったのでコメントアウトしていた部分を戻した。
  • またbinsonとの差分が0であることを確認するテストがあったが、本変更により一致することはなくなり役目を終えたとしてspecそのものを削除

@alitaso345 alitaso345 marked this pull request as ready for review June 11, 2023 04:47
@@ -1740,4 +1742,4 @@ YYLTYPE yylloc = yyloc_default;
<%# b4_percent_code_get([[epilogue]]) -%>
#line <%= output.aux.epilogue_first_lineno - 1 %> "<%= output.grammar_file_path %>"

<%= output.aux.epilogue -%>
<%= output.aux.epilogue -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a newline here? There is a newline on the end of this file in ruby/ruby (e1ccb2838b65f23e7a21bdd416160d03dd73e7f8).

--- a/ruby/ruby/tool/lrama/template/bison/yacc.c
+++ b/template/bison/yacc.c
@@ -1742,4 +1742,4 @@ yyreturnlab:
 <%# b4_percent_code_get([[epilogue]]) -%>
 #line <%= output.aux.epilogue_first_lineno - 1 %> "<%= output.grammar_file_path %>"

-<%= output.aux.epilogue -%>
+<%= output.aux.epilogue -%>
\ No newline at end of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yui-knk Sure! I added a newline and fixup in c705968

Template file on ruby/ruby/tool/lrama was changed by ruby/ruby@bdaa491 .
Ported changes from ruby/ruby to ruby/lrama.
@alitaso345 alitaso345 force-pushed the alitaso345/add_user_argument branch from f1398e8 to b5c19cb Compare June 11, 2023 06:12
@alitaso345 alitaso345 requested a review from yui-knk June 11, 2023 06:16
@yui-knk yui-knk merged commit 8d376d5 into ruby:master Jun 11, 2023
@yui-knk
Copy link
Collaborator

yui-knk commented Jun 11, 2023

Thanks a lot!

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