<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128016/">https://git.reviewboard.kde.org/r/128016/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 26th, 2016, 10:56 a.m. CEST, <b>Sven Brauch</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="https://git.reviewboard.kde.org/r/128016/diff/1/?file=465464#file465464line99" style="color: black; font-weight: bold; text-decoration: underline;">sublime/idealdockwidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">99</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">menu</span><span class="p">.</span><span class="n">addSection</span><span class="p">(</span><span class="n">windowIcon</span><span class="p">(),</span> <span class="n">m_view</span><span class="o">-></span><span class="n">document</span><span class="p">()</span><span class="o">-></span><span class="n">title</span><span class="p">());</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">99</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">menu</span><span class="p">)</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Don't do this. It just clutters the code, and you would have to fix it in about every second line of the application anyways, should it ever happen (it doesn't on linux).</p></pre>
</blockquote>
<p>On May 26th, 2016, 11:08 a.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't understand what you're implying here. Not doing this would either mean not using a pointer at all (= abandoning the patch), or not checking if the allocation succeeded. And that's not acceptable either. No matter how likely an application is to crash anyway if an allocation fails.
>From what I see this is true also when using a QScopedPointer.</p></pre>
</blockquote>
<p>On May 26th, 2016, 11:46 a.m. CEST, <b>Sven Brauch</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Nowhere in KDE code does anybody ever check if an allocation succeeds. IMO it's pedantic and useless. We are not writing life-critical software; if your system is out of memory, your IDE crashes, such is life. On Linux it doesn't do anything <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">anyways</em> in the default kernel configuration, since allocations always succeed.</p></pre>
</blockquote>
<p>On May 26th, 2016, 12:22 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I just don't agree with that. If you really insist on not checking here, I'd propose that you remove the 2 lines in a separate commit, because I don't want to go on permanent record not doing things properly.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Writing proper code is indeed pedantic. Is it useless? What's the percentage of security fixes that we would NOT see if everyone always checked against NULL before using a pointer? </p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">allocations always succeed.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not sure if I should be in awe for a kernel that apparently is able to provide more memory than I could ever afford to buy, or lose quite a bit of esteem for inciting lazy and sloppy programming practices...</p></pre>
</blockquote>
<p>On May 26th, 2016, 2:01 p.m. CEST, <b>Kai Uwe Broulik</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If operator new fails it throws std::bad_alloc in which case your program will blow up anyway as we don't catch exceptions anywhere.</p></pre>
</blockquote>
<p>On May 26th, 2016, 2:28 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tomaz posted a more detailed comment with the same gist on the ML (Tomaz: please consider replying via the website, it's twice now that I scanned in vain through the RR trying to find your contributions ;) )</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, that's more constructive, and apparently the Linux kernel has nothing to do with it. I clearly never absorbed the fact that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">new</code> throws in case of failure, I guess I've never been sufficiently annoyed by that yet :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the check is pointless I'll remove it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That doesn't mean I consider it pointless to check, or handle allocation exceptions. I just hope there are no situations in which a big allocation fails that could have been handled with a nice "sorry, can't do that right now" failure message rather than a downright abort which can evidently lead to data (code) loss.</p></pre>
</blockquote>
<p>On May 26th, 2016, 5:44 p.m. CEST, <b>Sven Brauch</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I clearly never absorbed the fact that new throws</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Because it doesn't, unless you set vm.overcommit_memory to 2 by hand in sysctl. :D</p></pre>
</blockquote>
<p>On May 26th, 2016, 6:01 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's certainly not the reason. It's just one of those facts that I forgot I knew on a conscious level.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't see the fun in that overcommit thing at all, to be honest. A quick search turned up an article I couldn't agree more with: http://www.etalabs.net/overcommit.html</p></pre>
</blockquote>
<p>On May 26th, 2016, 6:03 p.m. CEST, <b>Christoph Cullmann</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You can not sanely handle OOM anyways in any GUI application.
Even if you clobber all your code with if's, the toolkits will all die anyway, if you OOM.
Beside safety critical stuff that flys your airplane and such the world has given up to handle OOM.</p></pre>
</blockquote>
<p>On May 26th, 2016, 6:15 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's not an excuse to be sloppy IMHO, esp. not in code that's supposed to be cross-platform.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How are you supposed to find out that you're trying to allocate something with a negative (or insanely big) number of elements, for instance?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And I certainly hope that the hardened/industrial kernels used in industrial and mission-critical applications use few as possible fancy gimmicks that have the term <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">heuristic</em> in their description!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://www.win.tue.nl/~aeb/linux/lk/lk-9.html#ss9.6</p></pre>
</blockquote>
<p>On May 26th, 2016, 6:20 p.m. CEST, <b>Christoph Cullmann</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's not an excuse to be sloppy IMHO, esp. not in code that's supposed to be cross-platform.
That is not sloppy. Its just a design decision: if you are OOM, you are doomed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How are you supposed to find out that you're trying to allocate something with a negative (or insanely big) number of elements, for instance?
Check what you pass to your allocation function, if your code doesn't check e.g. length before some malloc (lenght * ....) than that is sloopy.
To rely on malloc for that is no good idea, you might even get memory for "large amounts" and drive your system to swap which is more or like like "death"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And I certainly hope that the hardened/industrial kernels used in industrial and mission-critical applications use few as possible fancy gimmicks that have the term heuristic in their description!
If you do hard realtime (which I analyse for work), you follow stuff like this for C++: http://www.stroustrup.com/JSF-AV-rules.pdf</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">AV Rule 206 (MISRA Rule 118, Revised)
Allocation/deallocation from/to the free store (heap) shall not occur after initialization. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">;=) But that is just a funny sidenote :P</p></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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Its just a design decision: if you are OOM, you are doomed.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, that's where I don't agree. There are plenty of situations where whatever you wanted to do is doomed (like that ANOVA on a few gigabytes of data), but none of the rest if the code is written correctly (and gets the chance to do all the checks we dinosaurs are accustomed to write). Granted, that's the kind of situation only developers who use their own code for real work will ever see O:-)</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Check what you pass to your allocation function [...]</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of course, but who hasn't had to realise s/he forgot that, or refactored some code without adding the check, or simply assumed that "size_t cannot ever go negative" (but of course it can become huge)? That's not a big deal if the culprit is simply killed (or slaps a warning in your face and then exits gracefully). It's a lot more annoying if it takes down half or your login session with it (or worse) just because the kernel <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">guessed</em> wrong who to kill a couple of times.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[OT]
OS X doesn't have this kind of annoyance (it has others, like suspending CPU hogs). But it rarely if ever runs out of memory once you have a reasonable amount of RAM for the kind of use you have (12Gb in my case). It rarely even gets to swap because it has a memory compression feature that works very nicely. (And a kind of garbage collector that you can apparently trigger by simply allocating the remaining free space)</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Allocation/deallocation from/to the free store (heap) shall not occur after initialization. </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That sounds more or less like what you get on a good old OS-less microcontroller (which never crashes, btw, according to a former boss :))</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[/OT]</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On May 26th, 2016, 5:34 p.m. CEST, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated May 26, 2016, 5:34 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OS X can be capricious when instances corresponding to a widget are deleted, if the class in question uses "native" ObjC SDKs behind the scenes. Pending events can in that case be (generated and) delivered to objects that were already deleted.
According to the documentation, one should prefer to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QObject::deleteLater()</code> rather than the regular, direct <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete</code> whether it be explicit or implicit.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've long used a local patch that uses this approach in order to prevent a recurring crash after using the context menu of the "ideal dock widget". Somehow I never put it up for review here, apparently.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Builds and permits reliable behaviour on both OS X and Linux.</p></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>sublime/idealdockwidget.cpp <span style="color: grey">(dae0ea2)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128016/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>