Review Request: Convenience KRandom methods

[Sebastian Trueg]

> 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.
> 
> Sebastian Trueg wrote:
>     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.
> 
> Ingo Klöcker wrote:
>     I repeat my question: Why re-invent the wheel? I really don't understand the aversion of many KDE developers against using boost. KDE is mostly written in C++. So why not use some of the best free C++ libraries existing on this planet?
> 
> Thomas Lübking wrote:
>     No idea. But random thoughts?
>     - boost is pretty large and this item is pretty small?
>     - it itersects with Qt
>     - it intersects with c++0x (since it partially became part of it, including this item, i think - not sure, though)
>     (- on windows, M$ has a competitive TR1 implementation... but that doesn't count :)
> 
> Ingo Klöcker wrote:
>     Yeah. I guess many KDE developers have the same misconceptions about boost.
>     
>     > - boost is pretty large and this item is pretty small?
>     
>     Currently, this item is small. Next somebody will want random numbers that are distributed normally. In any case, you don't have to use all of boost.
>     
>     > - it itersects with Qt
>     
>     So what. Simply leave out those parts of boost that Qt provides (or tries to provide).
>     
>     > - it intersects with c++0x
>     
>     See above. However, C++0x won't be usable by us within the next few years unless we raise the required compilers to the latest versions. So this makes this point moot. Or, in fact, it is an argument in favor of using boost because we can use boost now and then switch to using the C++0x equivalents when we can use C++0x.

I personally have no problem whatsoever with using boost. Also because rather simple parts of boost like this are header-based. Thus, the overhead for kdelibs is minimal and should be very easy to make optional since we can simply use the ugly one I proposed as a fallback.


- 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/20100816/4975bc94/attachment.htm>