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

Revert "use generics in random (#16283)" #16291

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

ringabout
Copy link
Member

This reverts commit 71e2a9e.

@ringabout ringabout marked this pull request as draft December 8, 2020 10:25
@cooldome
Copy link
Member

cooldome commented Dec 8, 2020

Please leave description on why. Useful for history records at least.

@timotheecour
Copy link
Member

timotheecour commented Dec 8, 2020

[EDIT]

the reason is #16283 introduced undesirable breaking changes as was discovered during post review of #16283 and #16288:

import std/random
proc main()=
  type Foo = 1..5
  for i in 0..<10:
    let b = rand(Foo.high)
    echo (b, $b.type)
main()

1.4.0:
(5, "int")
(0, "int")
...
current devel 1.5.1 2297b96:
(5, "Foo")
Error: unhandled exception: value out of range: 0 notin 1 .. 5 [RangeDefect]

proposal

this will make everything sane again, and makes 0 compromises on features:

the API prior to #16283 was sane, impossible to misuse, because the input range matched the output range:
proc rand*(max: int): int
proc rand*(max: float): float =

after #16283 it stopped being sane and has either unfixable edge cases (because input range != output range), or breaks existing valid code.

So I recommend:

After that we can fix the remaining issues:

Note that rand(HSlce) allows doing everything you want (eg: rand(0..uint64.high) or rand(MyEnum.low .. MyEnum.high)), so it turns out we actually don't need to support rand(uint64.high)

@ringabout ringabout marked this pull request as ready for review December 9, 2020 01:30
@ringabout ringabout merged commit a32acc3 into nim-lang:devel Dec 9, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants