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

Merged
merged 3 commits into from Jan 26, 2018

Conversation

Projects
None yet
3 participants
@yui-knk
Contributor

yui-knk commented Jan 25, 2018

  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 yui-knk changed the title from Remove `f_to_r` call to Fix `convertCommon` Jan 25, 2018

Fix `convertCommon`
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
@enebo

This comment has been minimized.

Member

enebo commented Jan 25, 2018

@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.

@yui-knk

This comment has been minimized.

Contributor

yui-knk commented Jan 26, 2018

@enebo I fixed regressions. The rest of failures are encoding related test cases, which also fail on the head of jruby-9.1:

  • TestTranscode#test_windows_1255
  • Integer#chr without argument when Encoding.default_internal is not nil and self is greater than 255 raises RangeError if self is invalid as a codepoint in the default internal encoding
  • Integer#chr with an encoding argument raises RangeError if self is invalid as a codepoint in the specified encoding FAILED

@enebo enebo added this to the JRuby 9.1.16.0 milestone Jan 26, 2018

@enebo enebo added the core label Jan 26, 2018

@enebo enebo merged commit d26cad1 into jruby:jruby-9.1 Jan 26, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@enebo

This comment has been minimized.

Member

enebo commented Jan 26, 2018

@yui-knk jcodings was updated yesterday and is the fallout. New updated data for encoding support. The regressions need to be examined still.

@yui-knk yui-knk deleted the yui-knk:rational_convert branch Jan 27, 2018

@kares

This comment has been minimized.

Member

kares commented Jan 30, 2018

not sure about these changes - I think we might have clashed on blindly following MRI
that f_to_r with the "Fix spec:ruby:fast" msg doesn not tell much and is not part of the PR's desc

+ there's a Rational regression (test failing) on master, @yui-knk not sure but might be relate to these?
or maybe do you recall if that f_to_r is necessary (a legit fix)?
(was actually doing the opposite on master - trying to avoid the dynamic call-site when not needed - for a rational instance having to need to go through to_r just seems like smt I would try to avoid 😈 )

@enebo

This comment has been minimized.

Member

enebo commented Jan 30, 2018

@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?

@kares

This comment has been minimized.

Member

kares commented Jan 30, 2018

sorry, the first commit is nice and clear but the following isn't ... that is what I was hoping to understand why
anyway I will check it later whether its related to the failing test on master

@yui-knk

This comment has been minimized.

Contributor

yui-knk commented Jan 31, 2018

The second commit is needed to fix

  • "Numeric#denominator works with Numeric subclasses" in spec/ruby/core/numeric/denominator_spec.rb
  • "Numeric#numerator works with Numeric subclasses" in spec/ruby/core/numeric/numerator_spec.rb

These specs expect "#to_r" is called when #denominator/#numerator is called for Numeric subclass.

By the way #5022 will fix failing test on master.

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