-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Fixnum does not allow overloading the case keyword implementation using === #1486
Comments
Verified that in a test. Seems like Fixnums are being handled differently, probably for optimizations. Right now I'm trying to pinpoint the culprit. I want to blame https://github.com/jruby/jruby/blob/jruby-1_7/core/src/main/java/org/jruby/ast/WhenOneArgNode.java#L48 (since FixNum implements IEqlNode) but commenting that out seems to not quite work. Debugging further... |
Unfortunately, seems like now comparing two fixnums will incur a regular ruby object comparison overhead. But that seems unavoidable when we want to allow to override behavior of even core classes.
Duh, that was indeed the culprit, it's just that my test was confusing me. Anyhow, PR sent out, but seems like as is it will cause performance regression, since when comparison will always use the "slow" ruby comparison. But then that's how multiple argument When behaves, so I think it's okay. |
First of all... why do you want to do this? Second, I came up with a patch that doesn't hurt the fast path as much, but this – like @ratnikov's patch – still only fixes the AST interpreter (not the AST JIT): https://gist.github.com/headius/a93ad238c0b661f62f13 I'm not convinced that fixing this is actually worthwhile. |
We generally like to be compatible but this is one of those corners we do not want to put any effort into it unless there is a good use case. The performance benefits of not supporting this right now outweighs the impl cost of supporting an uncommon feature... |
I'm trying to redefine #===.
Here is the test case:
On MRI 2.0.0-p0 this works and prints "YES". I tested on Jruby 1.7.3 and 1.7.10 and they both printed "NO". I need this feature to be able to support a simpler syntax in my code (of course, the example above is overly simplified and does not reflect my end use case).
Note that calling 3 === 4 DOES return true for both Ruby and JRuby. However, the "case" statement does not use the redefined #=== method for Fixnum's. So far I have only seen this issue with Fixnum's; I know that String works without issues. You may wish to test this on other classes as well (e.g. Numeric's and Integer's).
EDIT: corrected typo. (MRI prints "YES" and Jruby prints "NO" instead of the other way around.)
The text was updated successfully, but these errors were encountered: