Random with range #1875

Merged
merged 2 commits into from Aug 27, 2012

Conversation

Projects
None yet
3 participants
Contributor

LTe commented Aug 24, 2012

MRI:

rand(0.25..0.75) #=> 0.6428937309734553 

Rubinius before patch:

rand(0.25..0.75)
ArgumentError: invalid argument - 0.5
    from kernel/common/random19.rb:19:in `random'
    from kernel/common/random19.rb:8:in `random_range'
    from kernel/common/kernel19.rb:51:in `rand'
    from (irb):1
    from kernel/common/block_environment.rb:75:in `call_on_instance'
    from kernel/common/eval.rb:75:in `eval'
    from kernel/common/kernel19.rb:42:in `loop'
    from kernel/common/throw_catch19.rb:8:in `catch'
    from kernel/common/throw_catch.rb:10:in `register'
    from kernel/common/throw_catch19.rb:7:in `catch'
    from kernel/common/throw_catch19.rb:8:in `catch'
    from kernel/common/throw_catch.rb:10:in `register'
    from kernel/common/throw_catch19.rb:7:in `catch'
    from kernel/delta/codeloader.rb:68:in `load_script'

Rubinius after patch

rand(0.25..0.75) # => 0.6086817891310674 

This pull request fails (merged b3503128 into dd510a8).

Owner

dbussink commented Aug 24, 2012

Did you actually run all the specs? Looks like this change breaks the build

This pull request fails (merged ecded34f into dd510a8).

Contributor

LTe commented Aug 24, 2012

Looks like this change breaks the build

Fixed. Now travis return

Error: #<NativeException: org.virtualbox_4_1.VBoxException: The function "powerDown" returned an error condition: "The virtual machine is being powered down"  (0x80bb0002)>

This pull request passes (merged 01b799d2 into 9bb0a65).

@dbussink dbussink and 1 other commented on an outdated diff Aug 26, 2012

kernel/common/random19.rb
@@ -16,7 +16,7 @@ def random(limit)
random_range(limit)
else
limit_int = Rubinius::Type.coerce_to limit, Integer, :to_int
- raise ArgumentError, "invalid argument - #{limit}" if limit_int <= 0
+ raise ArgumentError, "invalid argument - #{limit}" if limit_int <= 0 && limit <= 0
@dbussink

dbussink Aug 26, 2012

Owner

Isn't the first check now duplicate since if the last one matches, the first one has to match too by definition?

@LTe

LTe Aug 27, 2012

Contributor

I agree. But with this change I need also little modify one spec. Check out new version :)

This pull request passes (merged 46cccc6f into c32cafe).

Owner

dbussink commented Aug 27, 2012

Does this spec change still make it pass on MRI too?

Contributor

LTe commented Aug 27, 2012

Does this spec change still make it pass on MRI too?

No, because MRI does not execute <= method

Owner

dbussink commented Aug 27, 2012

Then the spec is problematic and we can't spec it like this. I think we need to explicitly check here for a float type. You should probably check how MRI does this exact coercion here so we can match the behavior.

Contributor

LTe commented Aug 27, 2012

MRI check each type and try generate random number from each type (click)

  • if not float, try to_int and generate rand
  • if float -> generate rand

This pull request passes (merged dc43f492 into c32cafe).

Owner

dbussink commented Aug 27, 2012

Much better!

@dbussink dbussink added a commit that referenced this pull request Aug 27, 2012

@dbussink dbussink Merge pull request #1875 from LTe/random_range
Random with range
8cf8366

@dbussink dbussink merged commit 8cf8366 into rubinius:master Aug 27, 2012

1 check was pending

default The Travis build is in progress
Details

This pull request passes (merged 5e927f6 into c32cafe).

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