Skip to content

Random with range#1875

Merged
dbussink merged 2 commits intorubinius:masterfrom
LTe:random_range
Aug 27, 2012
Merged

Random with range#1875
dbussink merged 2 commits intorubinius:masterfrom
LTe:random_range

Conversation

@LTe
Copy link
Copy Markdown
Contributor

@LTe 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

@travisbot
Copy link
Copy Markdown

travisbot commented Aug 24, 2012

This pull request fails (merged b3503128 into dd510a8).

@dbussink
Copy link
Copy Markdown
Contributor

dbussink commented Aug 24, 2012

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

@travisbot
Copy link
Copy Markdown

travisbot commented Aug 24, 2012

This pull request fails (merged ecded34f into dd510a8).

@LTe
Copy link
Copy Markdown
Contributor Author

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)>

@travisbot
Copy link
Copy Markdown

travisbot commented Aug 26, 2012

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

dbussink
dbussink reviewed Aug 26, 2012
View reviewed changes
Comment thread kernel/common/random19.rb Outdated
Copy link
Copy Markdown
Contributor

@dbussink dbussink Aug 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@LTe LTe Aug 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@travisbot
Copy link
Copy Markdown

travisbot commented Aug 27, 2012

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

@dbussink
Copy link
Copy Markdown
Contributor

dbussink commented Aug 27, 2012

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

@LTe
Copy link
Copy Markdown
Contributor Author

LTe commented Aug 27, 2012

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

No, because MRI does not execute <= method

@dbussink
Copy link
Copy Markdown
Contributor

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.

@LTe
Copy link
Copy Markdown
Contributor Author

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

@travisbot
Copy link
Copy Markdown

travisbot commented Aug 27, 2012

This pull request passes (merged dc43f492 into c32cafe).

@dbussink
Copy link
Copy Markdown
Contributor

dbussink commented Aug 27, 2012

Much better!

dbussink added a commit that referenced this pull request Aug 27, 2012
@dbussink
Merge pull request #1875 from LTe/random_range
8cf8366 
Random with range
@dbussink dbussink merged commit 8cf8366 into rubinius:master Aug 27, 2012
@travisbot
Copy link
Copy Markdown

travisbot commented Aug 27, 2012

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LTe @travisbot @dbussink