Bug 11758 – std.random.uniform fails when mixing signed/unsigned integrals

Status
NEW
Severity
normal
Priority
P3
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2013-12-17T13:09:44Z
Last change time
2024-12-01T16:19:35Z
Assigned to
No Owner
Creator
Chris Cain
Moved to GitHub: phobos#9620 →

Comments

Comment #0 by zshazz — 2013-12-17T13:09:44Z
It appears that, despite accepting mixed signed/unsigned numbers, std.random.uniform fails to handle them properly. --- void main() { writeln(uniform(-1, 1u)); // std.random.uniform(): invalid bounding interval [-1, 1) foreach(_; 0..5) { writeln(uniform(-2, uint.max)); // Always prints out 4294967294 } } --- This is problematic to fix. Possible fixes and a counter argument for each: 1. Rejecting mixing signed/unsigned numbers. Issue: It may break a lot of code (std.random.randomShuffle, for instance, uses `uniform(0, someRange.length))`) 2. Accepting mixing signed/unsigned numbers, but upsizing the return type to the next largest signed integral type (so, promote byte/ubyte mixing to short, short/uint mixing to long, etc.) and rejecting cases where promotion is impossible (really just when one of the arguments is a long or ulong). This would allow things like `uniform(int.min, uint.max)` to be meaningful and actually return correct values. Issue: Would still break std.random.randomShuffle and other code. Might work on some code in 32-bit but cause that code to fail on 64-bit. It could also make it so that error messages differ between 32-bit and 64-bit (changing where failing code fails at, either at the usage of uniform or somewhere in client code). 3. Accepting mixing signed/unsigned numbers, but doing a run-time check that the signed number is non-negative. Issue: Disallows many legitimate use cases (for instance, `uniform(-2, 5u)` could return an int or long between -2 and 5 easily enough) unlike solution 2 and incurs an additional performance penalty for the check that would necessitate a recommendation to change existing code anyway, without a way to mechanically verify like the failure to compile as is in the case of solution 1. 4. Leave code as-is ... after all, it's been this way for quite awhile and no one has run into this problem. Issue: It's a silent ticking time bomb... Any suggestions for a path to fix? I think doing 1 but deprecating mixing signed/unsigned numbers for a few releases prior might be the best. It goes without saying that the new std.random could just do solution 1 and when code is migrated to the new std.random it can be fixed at that time.
Comment #1 by peter.alexander.au — 2014-02-23T09:41:44Z
I think 3 is the correct solution. You are right in saying that it disallows legitimate use cases such as "uniform(-2, 5u)", but currently these don't work, so we don't break any code. You are also right in saying that it incurs a small performance hit but (a) it is negligible compared to the generation of a random number, and (b) the user can easily fix it by not using mixed signed/unsigned.
Comment #2 by robert.schadek — 2024-12-01T16:19:35Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/phobos/issues/9620 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB