GNOME Bugzilla – Bug 760094
RollDie() can return 7.
Last modified: 2016-01-03 16:24:01 UTC
RollDie() is supposed to return a value between 1 and 6. rand()'s documentation says it returns 0..RAND_MAX **inclusive** [1]. So, value of r can be 1: double r = (double) rand () / RAND_MAX; Which will cause RollDie() to return 7: return (int) (r * 6.0) + 1; [1]: http://man7.org/linux/man-pages/man3/rand.3.html
Created attachment 318177 [details] [review] Fix RollDie(). Since rand() returns 0..RAND_MAX inclusive, RollDie() could return 7 if result of rand() equaled to RAND_MAX. This change makes sure the result of this function is in 1..6 range.
Thank you for spotting that and the patch. If possible, I would avoid any floating-point-math alltogether, once we're here. Do you see any issue in changing that to return rand () % 6 + 1;? The resulting "imbalance" of RAND_MAX being possibly not divisible by 6 should be negligible, right?
Also RAND_MAX might be MAX_INT so we shouldn't just add 1 to it, I guess.
Created attachment 318183 [details] [review] Fix RollDie(). Since rand() returns 0..RAND_MAX inclusive, RollDie() could return 7 if result of rand() equaled to RAND_MAX. This change makes sure the result of this function is in 1..6 range.
Created attachment 318184 [details] [review] Fix RollDie(). Since rand() returns 0..RAND_MAX inclusive, RollDie() could return 7 if result of rand() equaled to RAND_MAX. This change makes sure the result of this function is in 1..6 range.
Created attachment 318185 [details] [review] Fix RollDie(). Since rand() returns 0..RAND_MAX inclusive, RollDie() could return 7 if result of rand() equaled to RAND_MAX. This change makes sure the result of this function is in 1..6 range.
(In reply to Mario Wenzel from comment #2) > Thank you for spotting that and the patch. If possible, I would avoid any > floating-point-math alltogether, once we're here. Do you see any issue in > changing that to > > return rand () % 6 + 1;? Done. (Sorry for polluting this thread with multiple uploads. This is my first GNOME patch, so I made some errors to finally upload the correct patch) > > The resulting "imbalance" of RAND_MAX being possibly not divisible by 6 > should be negligible, right? Right. The previous implementation also had the imbalance. It also distributed 6k+r (r > 0) values into 6 bins. To make this completely uniform, I think we could do something like: int range_max = RAND_MAX - RAND_MAX % 6; int r = 0; while ((r = rand()) >= range_max); return r % 6 + 1; But I thought this might be an overkill here, because the error of current implementation is very small and it's much easier to understand.
Review of attachment 318185 [details] [review]: Fine :) Thank you