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 support for adjusting Index to Inline #394

Merged
merged 11 commits into from May 17, 2024
Merged

Conversation

ydah
Copy link
Collaborator

@ydah ydah commented Apr 9, 2024

No description provided.

@yui-knk
Copy link
Collaborator

yui-knk commented Apr 13, 2024

[note]
Some methods on RuleBuilder require parameterizing_rule_resolver for its argumen, e.g. #setup_rules, #has_inline_rules? and #resolve_inline_rules. It might be better to pass parameterizing_rule_resolver for RuleBuilder#initialize by adding helper function to Grammar which calls Grammar::RuleBuilder.new with @parameterizing_rule_resolver
It's not directly related to this PR, so I don't require to change it in this PR.

@@ -204,6 +207,9 @@ def replace_inline_user_code(inline_rhs, index)
return user_code if user_code.nil?

code = user_code.s_value.gsub(/\$#{index + 1}/, inline_rhs.user_code.s_value)
user_code.references[(index+1)..-1]&.each do |ref|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering several points for this method.

1. Order of user_code.references

The order of user_code.references is the order of reference appearance. For example if the code is $3 $2 $1 $$, the order of user_code.references is also [$3, $2, $1, $$]. This means user_code.references[(index+1)..-1] might not what we want to do. I think it will be like:

user_code.references.each do |ref|
  next if ref.index.nil? || ref.index <= index # nil is a case for `$$`
  ...
end

2. Type of reference

user_code.references includes both $n and @n, we might need to take care of @n in the future

3. How to overwrite references

Right now this code overwrite code string, however another approach is to dup the code and overwrite reference index like below.
Manipulating references is helpful if we handle named references.

        result = user_code.dup
        result.references[(index+1)..-1]&.each do |ref|
          ref.index += inline_rhs.symbols.count-1
        end
        result

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for your review!
I fixed 1 and 2 and added a commit.

However, for 3, I changed the way to write references overwrites, but I tried rewriting it like this:. However, when I wrote a test to check UserCode references in parser.spec, the index didn't seem to change.

def replace_inline_user_code(inline_rhs, index)
  return user_code if inline_rhs.user_code.nil?
  return user_code if user_code.nil?

  code = user_code.s_value.gsub(/\$#{index + 1}/, inline_rhs.user_code.s_value)
  replaced_code = Lrama::Lexer::Token::UserCode.new(s_value: code, location: user_code.location)

  replaced_code.references.each do |ref|
    next if ref.index.nil? || ref.index <= index # nil is a case for `$$`

    ref.index += inline_rhs.symbols.count
  end
  replaced_code
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I'm missing something, please point it out 🙏

@yui-knk
Copy link
Collaborator

yui-knk commented Apr 13, 2024

@ydah The direction seems good. Could you check the logic of reference overwrite, #394 (comment) ?

@ydah
Copy link
Collaborator Author

ydah commented Apr 15, 2024

@yui-knk Thank you for your review! I updated this PR.

@ydah
Copy link
Collaborator Author

ydah commented Apr 17, 2024

[note] Some methods on RuleBuilder require parameterizing_rule_resolver for its argumen, e.g. #setup_rules, #has_inline_rules? and #resolve_inline_rules. It might be better to pass parameterizing_rule_resolver for RuleBuilder#initialize by adding helper function to Grammar which calls Grammar::RuleBuilder.new with @parameterizing_rule_resolver It's not directly related to this PR, so I don't require to change it in this PR.

@yui-knk Add refactoring commit: 514e61c

@ydah
Copy link
Collaborator Author

ydah commented May 17, 2024

@yui-knk I rebased and fix conflict.

@yui-knk yui-knk merged commit 48c4434 into ruby:master May 17, 2024
16 checks passed
@yui-knk
Copy link
Collaborator

yui-knk commented May 17, 2024

LGTM!

@ydah ydah deleted the inline-index branch May 17, 2024 07:42
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.

None yet

2 participants