Bug 9339 – std.random.uniform!Enum should return random enum member

Status
RESOLVED
Resolution
FIXED
Severity
enhancement
Priority
P2
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2013-01-17T14:33:00Z
Last change time
2013-02-13T00:13:44Z
Keywords
pull
Assigned to
andrej.mitrovich
Creator
hsteoh

Comments

Comment #0 by hsteoh — 2013-01-17T14:33:56Z
Currently, std.random.uniform does not respect enum bounds: import std.random; import std.stdio; enum Fruit { Apple = 14, Orange = 27, Pear = 36, Mango = 47 } void main() { writefln("%d", Fruit.min); writefln("%d", Fruit.max); writeln(uniform!Fruit()); } Typical output: 14 47 cast(Fruit)-2046817621 It should at the very least respect the enum range defined by the enum's .min and .max properties (which in this case are 14 and 47, respectively). Ideally, it should only return one of the four possible values of Fruit, not anything outside the range of .min and .max, and not anything in between the four possible values.
Comment #1 by andrej.mitrovich — 2013-01-17T16:31:49Z
Would this suffice? auto uniform(T)() if (is(T == enum) && isIntegral!T || isSomeChar!T) { enum arr = [EnumMembers!T]; return randomSample(arr, 1); } (You would have to add a !is(T == enum) in the other template). Example: import std.random; import std.stdio; import std.traits; enum Fruit { Apple = 14, Orange = 27, Pear = 36, Mango = 47 } auto uniform(T)() if (is(T == enum) && isIntegral!T || isSomeChar!T) { enum arr = [EnumMembers!T]; return randomSample(arr, 1); } void main() { foreach (i; 0 .. 10) writeln(uniform!Fruit()); }
Comment #2 by hsteoh — 2013-01-17T16:40:15Z
Yeah, that will do. Except that the "enum arr = [EnumMembers!E];" line may run into issue 6057. :)
Comment #3 by bearophile_hugs — 2013-01-17T16:47:21Z
(In reply to comment #1) > auto uniform(T)() > if (is(T == enum) && isIntegral!T || isSomeChar!T) > { > enum arr = [EnumMembers!T]; > return randomSample(arr, 1); > } I think this is more efficient: T uniform(T)() if (is(T == enum) && isIntegral!T || isSomeChar!T) { static immutable T[EnumMembers!T.length] members = [EnumMembers!T]; return members[std.random.uniform(0, members.length)]; }
Comment #4 by bearophile_hugs — 2013-01-17T16:48:48Z
(In reply to comment #1) > enum arr = [EnumMembers!T]; Be very careful with enum arrays. They are very inefficient.
Comment #5 by hsteoh — 2013-01-17T16:52:13Z
(In reply to comment #3) [...] > I think this is more efficient: > > > T uniform(T)() > if (is(T == enum) && isIntegral!T || isSomeChar!T) > { > static immutable T[EnumMembers!T.length] members = [EnumMembers!T]; > return members[std.random.uniform(0, members.length)]; > } You're right, we want the array to be statically initialized. Does enum arr = [...] cause the code to create the array every time the function is called?
Comment #6 by andrej.mitrovich — 2013-01-17T17:01:43Z
(In reply to comment #4) > (In reply to comment #1) > > > enum arr = [EnumMembers!T]; > > Be very careful with enum arrays. They are very inefficient. Well, the compiler is very inefficient, static will do. > T uniform(T)() > if (is(T == enum) && isIntegral!T || isSomeChar!T) > { > static immutable T[EnumMembers!T.length] members = [EnumMembers!T]; > return members[std.random.uniform(0, members.length)]; > } That's not doing what was requested.
Comment #7 by andrej.mitrovich — 2013-01-17T17:08:20Z
As much as I'd love to make a pull for this I already know I'm going to run into Issue 6057 (which has a pull but needs a review).
Comment #8 by andrej.mitrovich — 2013-01-17T17:29:52Z
(In reply to comment #7) > As much as I'd love to make a pull for this I already know I'm going to run > into Issue 6057 (which has a pull but needs a review). Looks like I said the same thing as Comment #2. :p
Comment #9 by hsteoh — 2013-01-17T17:36:18Z
If you write "static arr = [EnumMembers!T];", you should be able to evade issue 6057.
Comment #10 by andrej.mitrovich — 2013-01-17T17:43:00Z
(In reply to comment #9) > If you write "static arr = [EnumMembers!T];", you should be able to evade issue > 6057. The issue is with unittests.
Comment #11 by hsteoh — 2013-01-17T21:15:26Z
I don't understand. If you use that line in uniform(), and it works, then unittests shouldn't have any problems either, no?
Comment #12 by andrej.mitrovich — 2013-01-17T21:21:50Z
(In reply to comment #11) > I don't understand. If you use that line in uniform(), and it works, then > unittests shouldn't have any problems either, no? The problem is the enum has to be hidden in a unittest block like so: version(unittest) { enum TestEnum { ... } } unittest { foreach (_; 0 .. 100) assert(uniform!TestEnum() == ...); } And this causes linking problems due to Issue 6057.
Comment #13 by hsteoh — 2013-01-17T21:35:46Z
Oh? This code compiles & links just fine: import std.random; import std.traits; E randomPick(E)() if (is(E == enum)) { static members = [ EnumMembers!E ]; return members[uniform(0, EnumMembers!E.length)]; } void main() { } unittest { enum Fruit { Apple = 12, Mango = 29, Pear = 72 }; foreach (_; 0 .. 100) { auto f = randomPick!Fruit(); assert(f == Fruit.Apple || f == Fruit.Mango || f == Fruit.Pear); } }
Comment #14 by bearophile_hugs — 2013-01-18T10:20:09Z
(In reply to comment #6) > > T uniform(T)() > > if (is(T == enum) && isIntegral!T || isSomeChar!T) > > { > > static immutable T[EnumMembers!T.length] members = [EnumMembers!T]; > > return members[std.random.uniform(0, members.length)]; > > } > > That's not doing what was requested. Then I don't understand. This ER asks for that function overload to return a "random enum member". Isn't members[std.random.uniform(0, members.length)] a random enum member? And beside what the OP is asking, uniform() returns single random values of a type. Isn't this what I am doing there?
Comment #15 by andrej.mitrovich — 2013-01-18T10:30:47Z
(In reply to comment #14) > (In reply to comment #6) > > > > T uniform(T)() > > > if (is(T == enum) && isIntegral!T || isSomeChar!T) > > > { > > > static immutable T[EnumMembers!T.length] members = [EnumMembers!T]; > > > return members[std.random.uniform(0, members.length)]; > > > } > > > > That's not doing what was requested. > > Then I don't understand. I've misread the last statement (it was late) as: return std.random.uniform(0, members.length); instead of: return members[std.random.uniform(0, members.length)]; Apologies.
Comment #16 by andrej.mitrovich — 2013-01-21T17:01:19Z
Comment #17 by bearophile_hugs — 2013-01-21T17:10:03Z
(In reply to comment #16) > https://github.com/D-Programming-Language/phobos/pull/1086 I have seen that of this line you have dropped both the immutable and fixed sized array, can you explain why a dynamic array is better than a fixed array in the static segment? static immutable T[EnumMembers!T.length] members = [EnumMembers!T];
Comment #18 by andrej.mitrovich — 2013-01-21T17:21:07Z
(In reply to comment #17) > (In reply to comment #16) > > https://github.com/D-Programming-Language/phobos/pull/1086 > > I have seen that of this line you have dropped both the immutable and fixed > sized array, can you explain why a dynamic array is better than a fixed array > in the static segment? > > static immutable T[EnumMembers!T.length] members = [EnumMembers!T]; It was a mistake, I've updated the code now.
Comment #19 by github-bugzilla — 2013-02-12T17:25:07Z
Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/c95100ba78cc137ee4af37d59e3b3dfb704a4832 Fixes Issue 9339 - Uniform for enums. https://github.com/D-Programming-Language/phobos/commit/6a3ffa5e136d22b31529e6a3688cb8ce3a5508a0 Merge pull request #1086 from AndrejMitrovic/Fix9339 Issue 9339 - std.random.uniform for enums
Comment #20 by bearophile_hugs — 2013-02-12T17:33:09Z
One test case to try: import std.bigint, std.random; enum Foo : BigInt { zero = BigInt(0), one = BigInt(1) } void main() { auto x = uniform!Foo(); }
Comment #21 by andrej.mitrovich — 2013-02-12T17:38:45Z
(In reply to comment #20) > One test case to try: > > > import std.bigint, std.random; > > enum Foo : BigInt { > zero = BigInt(0), > one = BigInt(1) > } > > void main() { > auto x = uniform!Foo(); > } It works. It should actually work with any enum base types, since the only requirement is that the type is an enum.
Comment #22 by github-bugzilla — 2013-02-13T00:13:44Z