Review Request: Convenience KRandom methods

[Sebastian Trueg]

> On 2010-08-12 19:17:04, Ingo Klöcker wrote:
> > trunk/KDE/kdelibs/kdecore/util/krandom.h, line 39
> > <http://reviewboard.kde.org/r/4992/diff/3/?file=33700#file33700line39>
> >
> >     This is wrong. Quoting the man page of rand (3):
> >     "The rand() function returns a pseudo-random integer in the range [0, RAND_MAX]."
> >     
> >     Note the closing ']'.

I just copied the existing comment. :)


> On 2010-08-12 19:17:04, Ingo Klöcker wrote:
> > trunk/KDE/kdelibs/kdecore/util/krandom.cpp, line 55
> > <http://reviewboard.kde.org/r/4992/diff/3/?file=33701#file33701line55>
> >
> >     This will probably crash if you call it with min = MIN_INT and max = MAX_INT because then max-min+1 = 0 (due to an integer overflow). In fact, even smaller ranges will most likely be problematic as soon as max-min+1 > MAX_INT.
> >     
> >     Apart from the integer overflow problem this method will not return uniformly distributed random numbers. See for example http://www.cplusplus.com/reference/clibrary/cstdlib/rand/.
> >     
> >     If you want uniformly distributed random numbers in a certain integer range then use
> >     http://www.boost.org/doc/libs/1_43_0/doc/html/boost_random/tutorial.html#boost_random.tutorial.generating_integers_in_a_range
> >     instead of trying to re-invent a wheel that is much more complicated than you think.
> >     
> >     As far as I'm concerned I'm against adding such a flawed method to kdelibs.

And this is exactly why it makes so much sense to put it in kdecode. There are thousands of ways to get this wrong. I only showed two so far. ;)
And btw: I do not think this is simple. That is why I want your feedback.
I will look at the boost approach and have another go at it.


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4992/#review7017
-----------------------------------------------------------


On 2010-08-12 13:40:52, Sebastian Trueg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4992/
> -----------------------------------------------------------
> 
> (Updated 2010-08-12 13:40:52)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The KRandom namespace is quite useful. However, applications need to be full of code snippets that calculate a random number in a range. IMHO it makes perfect sense to do this once in the library. Thus, I am proposing this patch to introduce two methods providing random numbers from a range.
> Feel free to improve the implementation. :)
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdecore/util/krandom.h 1162164 
>   trunk/KDE/kdelibs/kdecore/util/krandom.cpp 1162164 
> 
> Diff: http://reviewboard.kde.org/r/4992/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100812/eb330335/attachment.htm>