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.

Sign up for GitHub

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

Jump to bottom

Array inject accept strings #3425

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

Array inject accept strings #3425

wants to merge 2 commits into from

Conversation

wasabigeek
Copy link
Contributor

@wasabigeek wasabigeek commented Feb 1, 2024 •
edited

Attempt to address #2931. This expands the specialisations for InjectNode to accept strings in place of symbols.

Local test:

➜  truffleruby git:(master) ✗ jt ruby -e "p [2,3].inject('*')"
$ /.../ruby \
  --experimental-options \
  --core-load-path=/.../truffleruby/src/main/ruby/truffleruby \
  -e \
  p\ \[2,3\].inject\(\'\*\'\)
6
➜  truffleruby git:(master) ✗ jt ruby -e "p [2,3].inject(3, '*')"
Using Interpreted TruffleRuby: mxbuild/truffleruby-jvm
$ /.../truffleruby/mxbuild/truffleruby-jvm/languages/ruby/bin/ruby \
  --experimental-options \
  --core-load-path=/.../truffleruby/src/main/ruby/truffleruby \
  -e \
  p\ \[2,3\].inject\(3,\ \'\*\'\)
18

@oracle-contributor-agreement Oracle Contributor Agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Feb 1, 2024
@wasabigeek wasabigeek changed the title Array inject accept strings WIP: Array inject accept strings Feb 1, 2024
@wasabigeek wasabigeek marked this pull request as ready for review February 2, 2024 05:31
@wasabigeek wasabigeek changed the title WIP: Array inject accept strings Array inject accept strings Feb 2, 2024
@oracle-contributor-agreement Oracle Contributor Agreement
Copy link

Thank you for signing the OCA.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Feb 5, 2024
@eregon eregon self-assigned this Feb 5, 2024
eregon
eregon reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, I did an early review.

Some parameter names are rather confusing, could you try to clarify them? (some of them were already bad before though).

The major concern here is avoiding to increase the number of specializations.
I think we should introduce a IsSymbolOrString (inlined) node which accepts either and profiles accordingly, to use in the guards. That would avoid increasing the number of specializations.
Like ToStringOrSymbolNode but returning true/false.

Would you like to try that? Or should I do it?

src/main/java/org/truffleruby/core/array/ArrayNodes.java
loopProfile);
}

private Object injectSymbolHelper(VirtualFrame frame, RubyArray array, String symbolOrString,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private Object injectSymbolHelper(VirtualFrame frame, RubyArray array, String symbolOrString,
private Object injectSymbolHelper(VirtualFrame frame, RubyArray array, String method,

src/main/java/org/truffleruby/core/array/ArrayNodes.java
VirtualFrame frame,
RubyArray array,
Object initialOrSymbolOrString,
RubySymbol symbolOrString,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RubySymbol symbolOrString,
RubySymbol symbol,

(or method)

src/main/java/org/truffleruby/core/array/ArrayNodes.java
VirtualFrame frame, RubyArray array, Object initialOrSymbol, RubySymbol symbol, Nil block,
VirtualFrame frame,
RubyArray array,
Object initialOrSymbolOrString,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Object initialOrSymbolOrString,
Object initial,

src/main/java/org/truffleruby/core/array/ArrayNodes.java
VirtualFrame frame, RubyArray array, RubySymbol initialOrSymbol, NotProvided symbol, Nil block,
VirtualFrame frame,
RubyArray array,
RubySymbol initialOrSymbolOrString,
Copy link
Member

Choose a reason for hiding this comment

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

symbol or method

@wasabigeek
Copy link
Contributor Author

I'd like to give it a try! May take some time to get back to this, hope that's alright

@eregon
Copy link
Member

eregon commented Feb 6, 2024

Yes, that's fine, there is no urgency for this issue/PR.
If you need any help feel free to ask @nirvdrum or on the GraalVM Slack truffleruby channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@eregon eregon

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wasabigeek @eregon @rwstauner