Prepend must set method's "defined class" to the new bottom. #4250

merged 1 commit into from Oct 30, 2016


None yet

1 participant

headius commented Oct 26, 2016
@headius headius Prepend must set method's "defined class" to the new bottom.
headius commented Oct 26, 2016

Note that I could not find any code in MRI that does this method-table-walking, so I'm confused how they realign methods to the new hierarchy. This appears to pass specs, but travis is a bit red right now.

@headius headius referenced this pull request in freerange/mocha Oct 26, 2016

Mocha test failures when using JRuby #274

headius commented Oct 30, 2016

I'm going to merge this. The method-walking already was occurring and this just adds an additional bit of state.

I believe MRI does not need to walk the method table because their method structs do not contain the originating class. Instead, when they search the method table, they produce a separate struct specific to that method table and its host class. Basically, they lazily determine what we store as implClass based on where the method is actually found at runtime.

We may want to adopt this approach in the future. I believe this would help fix the problems we have with method table cloning + super going up the wrong hierarchy. cc @enebo @subbuss.

@headius headius added this to the JRuby milestone Oct 30, 2016
@headius headius merged commit f2b3a17 into jruby:master Oct 30, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
continuous-integration/appveyor/pr AppVeyor build failed
@headius headius deleted the headius:methods_have_owners branch Oct 30, 2016
@floehopper floehopper added a commit to freerange/mocha that referenced this pull request Oct 31, 2016
@floehopper floehopper TEMP: Add jruby-head to build matrix
This is to see whether jruby/jruby#4250 has solved the problem in #274.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment