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 unexpected java.lang.ArithmeticException when converting Rational to BigDecimal #4336

Merged
merged 1 commit into from Dec 5, 2016

Conversation

Projects
None yet
4 participants
@kirs
Contributor

kirs commented Nov 26, 2016

I'm not sure that catch (ArithmeticException e) is the best way to fix it, but at least this would emulate MRI behaviour (read #4324 for more context)

Fixes #4324

@enebo @headius

Fix unexpected java.lang.ArithmeticException
when converting Rational to BigDecimal
Fixes #4324
@olleolleolle

This comment has been minimized.

Show comment
Hide comment
@olleolleolle

olleolleolle Nov 29, 2016

Contributor

(Totally unrelated to your fix:) The appveyor CI job failed with this message: https://ci.appveyor.com/project/jnr/jruby/build/2880/job/shniu1cn7yluu99o#L1692

Java HotSpot(TM) Client VM warning: ignoring option MaxPermSize=2G;
  support was removed in 8.0
Error occurred during initialization of VM
Could not reserve enough space for 3145728KB object heap

Hm, perhaps MAVEN_OPTS -Xmx2G could be something?

Contributor

olleolleolle commented Nov 29, 2016

(Totally unrelated to your fix:) The appveyor CI job failed with this message: https://ci.appveyor.com/project/jnr/jruby/build/2880/job/shniu1cn7yluu99o#L1692

Java HotSpot(TM) Client VM warning: ignoring option MaxPermSize=2G;
  support was removed in 8.0
Error occurred during initialization of VM
Could not reserve enough space for 3145728KB object heap

Hm, perhaps MAVEN_OPTS -Xmx2G could be something?

@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 29, 2016

Contributor

@olleolleolle I think that the ignoring option is just a warning because it's also printed on successful test runs:

Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2G; support was removed in 8.0
Running org.jruby.embed.internal.BiVariableMapTest
Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.485 sec - in 

I'm more worried about other failures like uninitialized constant FFI::Platform. Perhaps we should create a separate issue to discuss ways how we can fix Appveyor.

Contributor

kirs commented Nov 29, 2016

@olleolleolle I think that the ignoring option is just a warning because it's also printed on successful test runs:

Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2G; support was removed in 8.0
Running org.jruby.embed.internal.BiVariableMapTest
Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.485 sec - in 

I'm more worried about other failures like uninitialized constant FFI::Platform. Perhaps we should create a separate issue to discuss ways how we can fix Appveyor.

@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 30, 2016

Contributor

@enebo @headius you guys have any thoughts on this fix?

Contributor

kirs commented Nov 30, 2016

@enebo @headius you guys have any thoughts on this fix?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Dec 5, 2016

Member

@kirs najs fix ... although it looks a bit scary on first sight :)
the docs on java.math.BigDecimal#divide says throws ArithmeticException - if the exact quotient does not have a terminating decimal expansion ... so this should be very decent.

Member

kares commented Dec 5, 2016

@kirs najs fix ... although it looks a bit scary on first sight :)
the docs on java.math.BigDecimal#divide says throws ArithmeticException - if the exact quotient does not have a terminating decimal expansion ... so this should be very decent.

@kares kares added this to the JRuby 9.1.7.0 milestone Dec 5, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Dec 5, 2016

Member

@kares if you reviewed this and are happy with it you can merge it. I felt out of depth on this one.

Member

enebo commented Dec 5, 2016

@kares if you reviewed this and are happy with it you can merge it. I felt out of depth on this one.

@kares kares merged commit ec525af into jruby:master Dec 5, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kirs kirs deleted the kirs:rational-to-bigdecimal-fix branch Dec 5, 2016

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