Skip to content
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 an edge case in gcd calculation #5444

Merged
merged 1 commit into from Nov 20, 2018

Conversation

Projects
None yet
2 participants
@ahorek
Copy link
Contributor

ahorek commented Nov 12, 2018

fixes

-9223372036854775808.gcd(-9223372036854775808)

should return 9223372036854775808 not -9223372036854775808

MRI uses FIXNUM_P macro instead of (x instanceof RubyFixnum), so -9223372036854775808 is properly delegated to a bignum logic

pavel

@ahorek ahorek force-pushed the ahorek:gcd_fix branch from caf7bff to e422017 Nov 13, 2018

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

Can we reduce the duplication here and introduce an equivalent to FIXNUM_P?

@headius headius added this to the JRuby 9.2.5.0 milestone Nov 20, 2018

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Nov 20, 2018

FIXNUM_P macro means a positive number in range 0...FIXNUM_MAX
but our i_gcd can handle negative numbers except both (x == Long.MIN_VALUE && y == Long.MIN_VALUE)
https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/util/Numeric.java#L514

any idea for a better name?

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

Hmm it occurs to me now this is a more general overflow problem, isn't it? Won't some values overflow into Long.MAX_VALUE+1 or +2 or so on?

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

Actually now see that you're covering the specific case where "x" is MIN_VALUE. I don't have any better suggestion for this. I think we'll merge what you have and then definitely need to put together some test cases.

@headius headius merged commit 8c8cfc1 into jruby:master Nov 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Nov 20, 2018

Long.MIN_VALUE is -9223372036854775808
but Long.MIN_VALUE is 9223372036854775807
so gcd(Long.MIN_VALUE, Long.MIN_VALUE) should be 9223372036854775808, but it can't be represented as long

see the related discussion about this with @kares
#5364 (comment)

mri is unaffected because it doesn't allow to pass negative numbers to i_gcd at all, but their implementation of i_gcd has the same limitation

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

@ahorek Ok I think we're on the same page now.

I tweaked your changes a bit and went with "isLongMinValue" for a utility function in 2c00b59.

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Nov 20, 2018

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.