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

array.c: eql? identity check if immediate value #1206

Closed
wants to merge 1 commit into from

Conversation

dwfait
Copy link

@dwfait dwfait commented Jan 18, 2016

I previously opened this: #1200

This PR introduces similar performance improvements for 'immediate' (Fixnum, floats, etc..) values, whilst not changing behaviour for Float::NAN or objects with redefined eql? methods.

caveat: This does change behaviour if you monkey patch the eql? method for an immedate value class, but if you're doing that, I imagine Array#eql? is going to be low down on your list of problems.

Benchmarks

Marked improvement for arrays of immediate values: https://gist.github.com/dwfait/da225dcc4687dcaff2a5

No apparent degradation of performance for arrays of objects / non-immediate values: https://gist.github.com/dwfait/9add4b6442897d68a2cd

Float::NAN

As mentioned, in this PR treatment of Float::NAN is as before:

irb(main):001:0> [Float::NAN].eql? [Float::NAN]
=> false

However, I did not create a test for this, because Matz has stated comparison of NaN to NaN is undefined: https://bugs.ruby-lang.org/issues/1720

I have however included a test which I believe addresses the core of that issue - of objects where eql? will return false, even to itself.

 * array.c (recursive_eql): Check for identity equality if the value
   being compared is an immediate value.
@k0kubun k0kubun changed the base branch from trunk to master August 15, 2019 17:58
@k0kubun
Copy link
Member

k0kubun commented Aug 17, 2019

It seems to have a conflict now. Could you rebase this from master?

@k0kubun
Copy link
Member

k0kubun commented Aug 19, 2019

Let me close this as it has not been updated for a while. Please reopen this after resolving conflicts. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants