Skip to content

Never use identity comparison for Ruby objects.#92

Merged
headius merged 1 commit intoruby:masterfrom
headius:no_identity_compare
Nov 19, 2018
Merged

Never use identity comparison for Ruby objects.#92
headius merged 1 commit intoruby:masterfrom
headius:no_identity_compare

Conversation

@headius
Copy link
Contributor

@headius headius commented Nov 19, 2018

Numeric objects in JRuby do not have guarantees of idempotence. In
this code, many places used object equality to compare two Fixnum
objects, a situation which is never guaranteed to work, and which
is much more likely to fail when JRuby's cache of low-valued
Fixnum objects is turned off as in jruby/jruby#5401. This patch
removes all such object identity comparisons and replaces them
with .equals calls.

Numeric objects in JRuby do not have guarantees of idempotence. In
this code, many places used object equality to compare two Fixnum
objects, a situation which is never guaranteed to work, and which
is much more likely to fail when JRuby's cache of low-valued
Fixnum objects is turned off as in jruby/jruby#5401. This patch
removes all such object identity comparisons and replaces them
with .equals calls.
@headius
Copy link
Contributor Author

headius commented Nov 19, 2018

This was likely an oversight when I did the original port; MRI can afford to do direct comparisons of Fixnum objects since they are idempotent (and not really objects).

@headius
Copy link
Contributor Author

headius commented Nov 19, 2018

This does not affect CI execution because it only fails in a very specific, experimental JRuby mode (-Xfixnum.cache=false) but it should be merged for the next release (even if #92 isn't done).

@headius
Copy link
Contributor Author

headius commented Nov 19, 2018

I have tested that this fixes jruby/jruby#5401 and that it does not regress any tests. Work to get JRuby running in CI will continue in #93.

@headius headius merged commit 134a35d into ruby:master Nov 19, 2018
@headius headius deleted the no_identity_compare branch November 19, 2018 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant