<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/118994/">https://git.reviewboard.kde.org/r/118994/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 5th, 2014, 1:36 p.m. CEST, <b>Arjun Ak</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/118994/diff/5/?file=287351#file287351line102" style="color: black; font-weight: bold; text-decoration: underline;">dolphin/src/dolphinrecenttabsmenu.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void DolphinRecentTabsMenu::rememberClosedTab(const KUrl& primaryUrl, const KUrl& secondaryUrl)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">89</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">delete</span> <span class="n">action</span><span class="p">;</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">99</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">delete</span> <span class="n">action</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;">Is it safe to 'delete' a QObject? shouldnt we be using deleteLater()</p></pre>
</blockquote>
<p>On July 6th, 2014, 12:57 p.m. CEST, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's no general rule.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Deleting a QObject is as un/safe as deleting anything else.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You must ensure that the referenced memory is not used afterwards, what makes deleting "stuff" in eventfilters and slots "risky" unless you suck the event (return true) or queue the slot/know that the emitting code is safe against lossing "this" object (ie. it's a "special" problem if you connect to an objects signal and delete that very object from the slot if the emitting code does not return after the emit but tries to access members)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This does however hold for every other non-local heap structure (ie. if you delete away an objects member w/o the object noticing it, you cause a dangeling member pointer)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">::deleteLater() bypasses <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">some</em> problems, but is no bullet proof deletion.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Eg. the code around the patch sets the local action pointer "0", what is completely pointless (and makes me worry somebody did sth. w/o any idea /what/ he's doing ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you need protection against randomly deleted objects, you've to use a "smart" pointer (QPointer) and then test whether the pointer ::isNull() if you're unsure about the objects state.</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Deleting the triggered action which is passed on by QMenu::triggered(QAction*) is safe.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
(There is also an unit test to ensure that it doesn't cause any crashes - see https://qt.gitorious.org/qt/qtbase/source/ff31090d07cbbb6f67d259438939e810a0baf67f:tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp#L112-836 for more details)</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;">local action pointer "0", what is completely pointless<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Yes it's pointless to set local pointers to 0 on immediate-return, but in this case it makes sense (non immediate-return) to set invalid pointers to 0 to minimize programming errors, because when someone extends this method in future and he wants to use the (possibly invalid) action pointer, it can cause problems when he doesn't notice that the pointer can be invalid. (and when you take a look at our code, we do this in multiple places where we delete local pointers in non immediate-return code paths).</p>
</blockquote></pre>
<br />
<p>- Emmanuel</p>
<br />
<p>On July 6th, 2014, 12:46 p.m. CEST, Arjun Ak 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 Dolphin and KDE Base Apps.</div>
<div>By Arjun Ak.</div>
<p style="color: grey;"><i>Updated July 6, 2014, 12:46 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=336818">336818</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-baseapps
</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;">Upon pressing CTRL+SHIFT+T, the most recently closed tab is reopened (with the help of Go->'Recently closed tabs' menu).</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>dolphin/src/dolphinmainwindow.h <span style="color: grey">(7c3bff4)</span></li>
<li>dolphin/src/dolphinmainwindow.cpp <span style="color: grey">(d9fe645)</span></li>
<li>dolphin/src/dolphinrecenttabsmenu.h <span style="color: grey">(34d4153)</span></li>
<li>dolphin/src/dolphinrecenttabsmenu.cpp <span style="color: grey">(a39f994)</span></li>
<li>dolphin/src/dolphinui.rc <span style="color: grey">(52826bb)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118994/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>