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

Test new recursion #3896

Merged
merged 1 commit into from May 19, 2016
Merged

Test new recursion #3896

merged 1 commit into from May 19, 2016

Conversation

@headius
Copy link
Member

@headius headius commented May 17, 2016

Dunno if anyone wants to review this (e.g. @enebo or @kares) but it appears to have passed all tests and eliminates some really ugly misfeatures of the original port.

Note that I have not removed the old impl in case an external library uses it.

@headius headius force-pushed the test-new-recursion branch from 9530b33 to b80b147 May 17, 2016
@headius
Copy link
Member Author

@headius headius commented May 17, 2016

I have squashed all commits into one, since they didn't really add anything interesting to history.

@kares
Copy link
Member

@kares kares commented May 18, 2016

looking good to me ... the new logic seems much easier to follow 👍

The old recursion guard (ported from MRI years ago) had a number
of flaws:

* It forced an object ID for every object encountered.
* It created transient objects for every recursive stack.
* It was not using identity hashing (#3887).
* It used a RubyHash internally, which has much more overhead than
  a typical JDK Map.

The new implementation largely follows the pattern of the original
but fixes all the above items. It passes all untagged specs.

See also #3884, which started this whole thing.
@headius headius force-pushed the test-new-recursion branch from b80b147 to dc7c489 May 18, 2016
@headius headius merged commit c4a013b into master May 19, 2016
0 of 4 checks passed
0 of 4 checks passed
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@headius headius deleted the test-new-recursion branch May 19, 2016
@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.