-
-
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
Fix convertCommon
#5010
Fix convertCommon
#5010
Conversation
2792a3d
to
ca0f447
Compare
1. Remove f_to_r call MRI does not call `to_r` when `a1` is not a float nor a string. Ref: https://github.com/ruby/ruby/blob/v2_3_0/rational.c#L2423-L2428 2. Call `#to_r` when `a1` is an integer Ref: https://github.com/ruby/ruby/blob/v2_3_0/rational.c#L2445-L2446
@yui-knk you have a few regressions in spec:ruby:fast involving this. They are sometimes a bit pedantic on Ruby dispatch but at least one error looks like it may be recursing a lot. |
65b2157
to
ca984cb
Compare
@enebo I fixed regressions. The rest of failures are encoding related test cases, which also fail on the head of jruby-9.1:
|
@yui-knk jcodings was updated yesterday and is the fallout. New updated data for encoding support. The regressions need to be examined still. |
not sure about these changes - I think we might have clashed on blindly following MRI + there's a Rational regression (test failing) on master, @yui-knk not sure but might be relate to these? |
@kares the line below basically explains that change: "MRI does not call to_r when a1 is not a float nor a string." @yui-knk seems to be following MRI logic exactly and this fixed an MRI test (unsurprisingly). As for master, I have not checked but I half wonder if perhaps 2.4 changed behavior (and we should not have merged this fix to master)? His PR actually removes call to_r in that particular case for 2.3.x, but original f_to_r code always performs the dyncall so maybe 2.4.x+ requires always dyncalling now? |
sorry, the first commit is nice and clear but the following isn't ... that is what I was hoping to understand why |
The second commit is needed to fix
These specs expect "#to_r" is called when By the way #5022 will fix failing test on master. |
Remove f_to_r call
MRI does not call
to_r
whena1
is not a float nor a string.Ref: https://github.com/ruby/ruby/blob/v2_3_0/rational.c#L2423-L2428
Call
#to_r
whena1
is an integerRef: https://github.com/ruby/ruby/blob/v2_3_0/rational.c#L2445-L2446