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

Do a fast dup of args when all symbols, due to kwarg processing. #5399

Merged
merged 3 commits into from Nov 1, 2018

Conversation

@headius
Copy link
Member

@headius headius commented Nov 1, 2018

Keyword argument processing currently mutates the incoming Hash,
calling delete as arguments are distributed to local variables.
This allows easy support for the keyword rest argument, since it
just gets whatever's left, but it forces us to make this defensive
copy. We also can skip some duping if the keyword arguments are
all literal and not just contained in an opaque hash. Of course
we really just need to wire up straight-through keyword args
dispatch.

Fixes #5267.

Keyword argument processing currently mutates the incoming Hash,
calling delete as arguments are distributed to local variables.
This allows easy support for the keyword rest argument, since it
just gets whatever's left, but it forces us to make this defensive
copy. We also can skip some duping if the keyword arguments are
all literal and not just contained in an opaque hash. Of course
we really just need to wire up straight-through keyword args
dispatch.

Fixes #5267.
@kares
Copy link
Member

@kares kares commented Nov 1, 2018

should distil a minimal reproducer from #5267 ... or at least a previously failing test for the change

@headius headius merged commit e1e28c8 into jruby:master Nov 1, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@headius headius deleted the headius:dup_kwargs branch Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants