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

Fixing RangeError with code from 1.7 branch #3200

Merged
merged 1 commit into from Aug 5, 2015

Conversation

bigsur0
Copy link
Contributor

@bigsur0 bigsur0 commented Jul 31, 2015

With my previous commit, RangeError: bignum too big to convert into `long' was the result when running (0...2**64).max

This commit ports handling for non-RubyFixnums from the 1.7.20 branch.

Addresses, #3118

.. follow-up on PR #3196

@kares
Copy link
Member

kares commented Jul 31, 2015

would be nice if it compiled and could we please have a test/spec this time as well, thanks!

@kares kares added this to the JRuby 9.0.1.0 milestone Jul 31, 2015
@bigsur0 bigsur0 force-pushed the r6p-patch-jruby-issue-3118-2 branch from d840ad5 to f34200c Compare July 31, 2015 17:03
@bigsur0
Copy link
Contributor Author

bigsur0 commented Jul 31, 2015

This compiles now. My bad for the missing }. Also I added a spec in my previous commit that should have broken the spec suite with the RangeError in question. I've compiled locally successfully but am unable to run specs under specs/ruby from my working copy directory. Do you have any tips on how to do that? Is there a doc describing how the ruby specs are meant to be used?

@bigsur0
Copy link
Contributor Author

bigsur0 commented Aug 3, 2015

@kares, i can't seem to understand the Travis failure above. Can you help with that? Also please see my previous question as it relates to the best way to run specs.

@kares
Copy link
Member

kares commented Aug 3, 2015

@r6p obviously previous spec did not broke the test suite - so we're still need a correct spec here, thanks!

bin/jruby -S rake spec:ruby it is ... try rvm use system and/or mvn clean package before rake

@bigsur0
Copy link
Contributor Author

bigsur0 commented Aug 3, 2015

@kares the fact that the ruby specs on master aren't failing is concerning to me. The same code path exercised via jirb fails. Thoughts?

cmd: bin/jirb
>> (0...2**64).max
RangeError: bignum too big to convert into `long'
    from org/jruby/RubyRange.java:662:in `max'
    from (irb):1:in `<eval>'
    from org/jruby/RubyKernel.java:978:in `eval'
    from org/jruby/RubyKernel.java:1291:in `loop'
    from org/jruby/RubyKernel.java:1098:in `catch'
    from org/jruby/RubyKernel.java:1098:in `catch'
    from bin/jirb:13:in `<top>'

With my previous commit, RangeError: bignum too big to convert into
`long' was the result when running (0...2**64).max

This commit ports handling for non-RubyFixnums from the 1.7.20 branch.

Addresses, jruby#3118
@bigsur0 bigsur0 force-pushed the r6p-patch-jruby-issue-3118-2 branch from f34200c to 34f5c8f Compare August 4, 2015 21:57
@bigsur0
Copy link
Contributor Author

bigsur0 commented Aug 4, 2015

@kares, I finally figured out where to put the test so that it would fail as a part of the build when running the following suite.

 GEM_PATH=lib/ruby/gems/shared bin/jruby -S -Prake -Dtask=test:mri

I added the spec and amended the commit on this branch. You'll see that the test passes below.

$ GEM_PATH=lib/ruby/gems/shared bin/jruby -X-C -r ./test/mri_test_env.rb test/mri/runner.rb -q -- test/mri/ruby/test_range.rb
Run options: -q --

# Running tests:

Finished tests in 1.733000s, 19.0421 tests/s, 189.8442 assertions/s.
33 tests, 329 assertions, 0 failures, 0 errors, 0 skips

ruby -v: jruby 9.0.1.0-SNAPSHOT (2.2.2) 2015-08-04 34f5c8f Java HotSpot(TM) 64-Bit Server VM 24.71-b01 on 1.7.0_71-b14 +jit [darwin-x86_64]

@kares
Copy link
Member

kares commented Aug 5, 2015

excellent, thank you!

kares added a commit that referenced this pull request Aug 5, 2015
Fixing RangeError with code from 1.7 branch
@kares kares merged commit c90b146 into jruby:master Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants