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

Don't dup every element in Enumerator#drop (1.7) #4226

Merged
merged 1 commit into from Oct 18, 2016

Conversation

Projects
None yet
3 participants
@iconara
Contributor

iconara commented Oct 13, 2016

This fixes #4218 in JRuby 1.7.

In 06f0441 (JRUBY-6892) Enumerable#drop was changed to call #dup on every element that ended up in the result, but that might not work. Not all objects correctly implement #dup, or are even allocatable.

#take was not changed, and looking at the differences between #take and #drop the only thing that stood out was that the signature of the #each call was different. Changing from Arity.NO_ARGUMENTS to Arity.ONE_REQUIRED make the issue that 06f0441 tried to fix go away.

Don't dup every element in Enumerator#drop
In 06f0441 (JRUBY-6892) Enumerable#drop was changed to call #dup on every element that ended up in the result, but that might not work. Not all objects correctly implement #dup, or are even allocatable.

#take was not changed, and looking at the differences between #take and #drop the only thing that stood out was that the signature of the #each call was different. Changing from Arity.NO_ARGUMENTS to Arity.ONE_REQUIRED make the issue that 06f0441 tried to fix go away.

@iconara iconara changed the title from Fix 4218 1.7 to Fix #4218 in JRuby 1.7 Oct 13, 2016

@iconara iconara changed the title from Fix #4218 in JRuby 1.7 to Don't dup every element in Enumerator#drop (1.7) Oct 13, 2016

@iconara iconara changed the base branch from master to 1_7_20 Oct 13, 2016

@iconara iconara changed the base branch from 1_7_20 to jruby-1_7 Oct 13, 2016

@iconara

This comment has been minimized.

Show comment
Hide comment
@iconara

iconara Oct 13, 2016

Contributor

I'm unsure which branch that this PR should target, please help me out. The fix should go into the next 1.7.x release.

I'm also unable to run the tests in spec/regression, so I actually don't know if this works (I'm expecting it to since it's exactly the same fix as in #4225 that targets 9K).

Contributor

iconara commented Oct 13, 2016

I'm unsure which branch that this PR should target, please help me out. The fix should go into the next 1.7.x release.

I'm also unable to run the tests in spec/regression, so I actually don't know if this works (I'm expecting it to since it's exactly the same fix as in #4225 that targets 9K).

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 16, 2016

Member

you got the right branch (jruby-1_7) ... somehow we do not have a travis-ci run for this PR not sure why.

Member

kares commented Oct 16, 2016

you got the right branch (jruby-1_7) ... somehow we do not have a travis-ci run for this PR not sure why.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 18, 2016

Member

I'll verify locally.

Member

headius commented Oct 18, 2016

I'll verify locally.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 18, 2016

Member

Seems fine locally. I'll merge.

Member

headius commented Oct 18, 2016

Seems fine locally. I'll merge.

@headius headius added this to the JRuby 1.7.27 milestone Oct 18, 2016

@headius headius merged commit 6cba9d3 into jruby:jruby-1_7 Oct 18, 2016

1 check failed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment