Add faster versions of `equals`, `is_eql` and `is_equal`

danielpclark / rutie

“The Tie Between Ruby and Rust.”

MIT License

940 stars 62 forks source link

Add faster versions of `equals`, `is_eql` and `is_equal` #107

Closed asppsa closed 4 years ago

asppsa commented 5 years ago

Hi there,

I'm just getting my feet wet with Rust/Rutie, so its entirely possible that this patch is nonsensical. Also, I'm not sure if I have put things in the right place, etc. Happy to make any changes you consider necessary!

The changes are:

Object::equals is now a wrapper around the rb_equal C function;
Object::is_eql is now a wrapper around the rb_eql C function;
Object::is_equal simply performs an == comparison on the underlying values, which is the same behaviour as the rb_obj_equal C function.

asppsa commented 5 years ago

OK, so in Ruby 2.4, rb_eql returns an int instead of a VALUE ...

calavera commented 4 years ago

This is a very good point. Hopefully it will get into master soon :pray:

danielpclark commented 4 years ago

I'm hesitant to allow Ruby versioning for different code paths. Once you travel down this road it grows into an unmaintainable beast. I have some code commented out in Rutie waiting for "End of Life" (our own EOL timing, not Ruby's… see below) of older Ruby versions before I can use Ruby's frozen feature in certain places.

Once Ruby 2.7 comes out we can stop officially supporting 2.4 as we only support the 3 most recent versions of Ruby. Then you won't need this Ruby version check. So you just need to wait until Christmas for adding this new implementation… or work with a fork of this project until then.

asppsa commented 4 years ago

Sounds reasonable! I'll rework this PR some time in the lead-up to xmas

asppsa commented 4 years ago

I've dropped the is_eql implementation from this PR, so it now works without the versioning. I'll try to remember to come back & open a follow-up PR once 2.7 gets released.

danielpclark commented 4 years ago

Feel free to re-add is_eql. I'll take the time to review once that's back in.

asppsa commented 4 years ago

K, I've reverted back to the original commit from August.