<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://reviewboard.kde.org/r/4992/">http://reviewboard.kde.org/r/4992/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 12th, 2010, 7:17 p.m., <b>Ingo Klöcker</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4992/diff/3/?file=33701#file33701line55" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdecore/util/krandom.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int KRandom::random( int min, int max )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">55</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span><span class="p">(</span> <span class="n">random</span><span class="p">()</span> <span class="o">%</span> <span class="p">(</span><span class="n">max</span><span class="o">-</span><span class="n">min</span><span class="o">+</span><span class="mi">1</span><span class="p">)</span> <span class="o">+</span> <span class="n">min</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On August 12th, 2010, 7:32 p.m., <b>Sebastian Trueg</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On August 13th, 2010, 6:46 p.m., <b>Ingo Klöcker</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On August 13th, 2010, 7:32 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :)</pre>
</blockquote>
<p>On August 13th, 2010, 8:18 p.m., <b>Ingo Klöcker</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Sebastian</p>
<br />
<p>On August 12th, 2010, 1:40 p.m., Sebastian Trueg wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Sebastian Trueg.</div>
<p style="color: grey;"><i>Updated 2010-08-12 13:40:52</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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. :)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/KDE/kdelibs/kdecore/util/krandom.h <span style="color: grey">(1162164)</span></li>
<li>trunk/KDE/kdelibs/kdecore/util/krandom.cpp <span style="color: grey">(1162164)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4992/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>