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

Fixes Fixnum#bitLength when the value is negative. #3831

Merged
merged 1 commit into from Apr 27, 2016

Conversation

Projects
None yet
4 participants
@lucasallan
Member

lucasallan commented Apr 27, 2016

Fixes #3829

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 27, 2016

Member

Hey Lucas! seems a bit wrong isn't it ? ... maybe some test-cases would help :

2.3.0 :002 > 112.bit_length
 => 7 
2.3.0 :003 > -112.bit_length
 => 7 
Member

kares commented Apr 27, 2016

Hey Lucas! seems a bit wrong isn't it ? ... maybe some test-cases would help :

2.3.0 :002 > 112.bit_length
 => 7 
2.3.0 :003 > -112.bit_length
 => 7 
@lucasallan

This comment has been minimized.

Show comment
Hide comment
@lucasallan

lucasallan Apr 27, 2016

Member

@kares

>> 112.bit_length
=> 7
>> -112.bit_length
=> 0

It seems to be working for me - I'll send a PR with tests to rubyspecs.

Member

lucasallan commented Apr 27, 2016

@kares

>> 112.bit_length
=> 7
>> -112.bit_length
=> 0

It seems to be working for me - I'll send a PR with tests to rubyspecs.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 27, 2016

Member

-112 should not have a bit length of 0 ... if it does at your MRI its platform dependent or a bug.
... if you think of it it would be a pretty weird outcome to expect all < 0 to have a bit length of 0

Member

kares commented Apr 27, 2016

-112 should not have a bit length of 0 ... if it does at your MRI its platform dependent or a bug.
... if you think of it it would be a pretty weird outcome to expect all < 0 to have a bit length of 0

@lucasallan

This comment has been minimized.

Show comment
Hide comment
@lucasallan

lucasallan Apr 27, 2016

Member

You're right, it seems pretty weird - I was assuming MRI was right - my bad. Done @kares

Member

lucasallan commented Apr 27, 2016

You're right, it seems pretty weird - I was assuming MRI was right - my bad. Done @kares

@kares kares merged commit 9b63036 into jruby:master Apr 27, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@kares kares added this to the JRuby 9.1.0.0 milestone Apr 27, 2016

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 27, 2016

Member

Great, thanks Lucas.

Member

kares commented Apr 27, 2016

Great, thanks Lucas.

@jkr2255

This comment has been minimized.

Show comment
Hide comment
@jkr2255

jkr2255 Apr 27, 2016

Thanks for improvement, but for some values still return different value from MRI.

In MRI implementation uses bit inversion, (-(2**x)).bit_lengthbecomes x in MRI (and x + 1 for this implementation)

jkr2255 commented Apr 27, 2016

Thanks for improvement, but for some values still return different value from MRI.

In MRI implementation uses bit inversion, (-(2**x)).bit_lengthbecomes x in MRI (and x + 1 for this implementation)

@lucasallan

This comment has been minimized.

Show comment
Hide comment
@lucasallan

lucasallan Apr 27, 2016

Member

@jkr2255 It will be fixed by #3833

Member

lucasallan commented Apr 27, 2016

@jkr2255 It will be fixed by #3833

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