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

use generics in random #16283

Merged
merged 2 commits into from
Dec 7, 2020
Merged

use generics in random #16283

merged 2 commits into from
Dec 7, 2020

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 7, 2020

@ringabout ringabout marked this pull request as draft December 7, 2020 12:20
@ringabout ringabout marked this pull request as ready for review December 7, 2020 13:24
## numbers returned from this proc will always be the same.
##
## This proc uses the default random number generator. Thus, it is **not**
## thread-safe.
Copy link
Member

Choose a reason for hiding this comment

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

What about this remark? Shouldn't we keep it?

Copy link
Member Author

@ringabout ringabout Dec 7, 2020

Choose a reason for hiding this comment

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

It is already kept

## This proc uses the default random number generator. Thus, it is **not**

rand(max: float) already has the same comments and this generic procs reuse that commentbefore.

@Araq Araq merged commit 71e2a9e into nim-lang:devel Dec 7, 2020
proc rand*[T: Ordinal or SomeFloat](max: T): T {.benign.} =
## Returns a random floating point number in the range `T(0) .. max`.
##
## Allowed types for `T` are integers, floats, and enums without holes.
Copy link
Member

Choose a reason for hiding this comment

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

this comment is redundant (and only approximately correct) and should be removed, it's already specified what T is in the generated docs:

image

furthermore, Ordinal is clickable (thanks to docgen) so users who see Ordinal for the 1st time can just click to see the definition, which is explained in 1 place, where it belongs (https://nim-lang.github.io/Nim/system.html#Ordinal) => "1 definition rule" for docs

note that the definition for https://nim-lang.github.io/Nim/system.html#Ordinal should be fixed via s/enums/enums without holes/ but that's a separate issue

proc rand*(max: float): float {.benign.} =
## Returns a random floating point number in the range `0.0..max`.
proc rand*[T: Ordinal or SomeFloat](max: T): T {.benign.} =
## Returns a random floating point number in the range `T(0) .. max`.
Copy link
Member

Choose a reason for hiding this comment

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

s/random floating point number/random number/

@timotheecour
Copy link
Member

timotheecour commented Dec 8, 2020

@xflywind thanks for tackling this; but this needs more care:

when defined case1:
  import std/random
  when defined case1a:
    # Error: type mismatch: got <Rand, Foo>
    type Fooa = enum k0,k1,k2
    echo rand(Fooa.high)
  when defined case1b:
    # Error: type mismatch: got <Rand, Dollar>
    type Dollar = distinct int
    echo rand(int.high.Dollar)
  when defined case1c: # ok
    echo rand(int64.high)
  when defined case1d:
    # Error: unhandled exception: value out of range [RangeDefect]
    echo rand(uint64.high)
  when defined case1e:
    # Error: type mismatch: got <Rand, char>
    echo (rand(char.high),)

in particular this should also be updated to be made generic:

proc rand*(r: var Rand; max: Natural): int {.benign.} =

and you can move the old underlying proc rand*(r: var Rand; max: Natural): int {.benign.} = implementation to some

proc randImpl(r: var Rand; max: Natural): int {.benign.} = ...

let f = rand(1.0)
## f = 8.717181376738381e-07
rand(state, max)
result = T(rand(state, max))
Copy link
Member

Choose a reason for hiding this comment

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

cast to avoid overflow errors

ringabout added a commit to ringabout/Nim that referenced this pull request Dec 8, 2020
ringabout added a commit that referenced this pull request Dec 9, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* use generics in random

* fix
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
* use generics in random

* fix
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