<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="https://git.reviewboard.kde.org/r/116747/">https://git.reviewboard.kde.org/r/116747/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 11th, 2014, 11:17 p.m. UTC, <b>Aleix Pol Gonzalez</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/116747/diff/1/?file=253404#file253404line228" style="color: black; font-weight: bold; text-decoration: underline;">src/kcompletionbox.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">protected:</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">212</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#ifndef KDE_NO_DEPRECATED</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;">I wouldn't leave the implementation here. Move it to the .cpp file, this way it can be changed in the future, if it's required for some reason.
Also there's a typo in the method name.</pre>
</blockquote>
<p>On March 12th, 2014, 9:14 a.m. UTC, <b>David Gil Oliva</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;">Alex Merry inlined deprecated methods in https://git.reviewboard.kde.org/r/116012 , so I thought that it was the way to go...</pre>
</blockquote>
<p>On March 12th, 2014, 12:13 p.m. UTC, <b>Alex Merry</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;">Well, there's a balance to be struck: putting them in the header ensures there is no runtime cost to programs that don't use the deprecated methods, as the code should be optimised away, that the library is always binary-compatible even if you compile it with deprecated code disabled (*_NO_DEPRECATED) and makes the header code document how to replace existing calls. The downsides are an inability to fix the code later and an inability to access members of a private d-pointer class. Neither of those are an issue here, as we're just renaming the method.
tl;dr: I disagree with Aleix, and think it should stay in the header.
Oh, and Aleix: could you please select the whole method when you're doing a comment like this, rather than just the first line? Otherwise it's a pain to see what you're referring to. Thanks :-)</pre>
</blockquote>
<p>On March 12th, 2014, 1:25 p.m. UTC, <b>Aleix Pol Gonzalez</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;">Well, I wouldn't bother about runtime penalty given that it's deprecated and we shouldn't be using it anyway. Also we can't make assumptions on how things are going to be optimized.
But it's ok, I don't think it's worth discussing further, I doubt this is going to be a problem in the future.</pre>
</blockquote>
<p>On March 12th, 2014, 1:45 p.m. UTC, <b>Alex Merry</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 mean the runtime penalty for things that *don't* use it. If it's header-only, it doesn't go in the library, so there is no load-time symbol-lookup penalty, and no code-size penalty. Admittedly, both of those are probably negligible...</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;">Thank you both of you for the useful hints. With this little discussion I have learned some criteria to decide what to do now and in the future.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 11th, 2014, 11:17 p.m. UTC, <b>Aleix Pol Gonzalez</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;">Have you looked through the uses of the "un-slotted" methods? (lxr.kde.org). Maybe there's a reason for that... :/</pre>
</blockquote>
<p>On March 12th, 2014, 9:14 a.m. UTC, <b>David Gil Oliva</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;">Maybe I'm totally wrong, but I can't imagine any way that a getter can be useful as a slot.
connect (widget, SLOT(valueChanged(), completionBox, SIGNAL(isTabHandling());
Does it make sense? ¿?:-/</pre>
</blockquote>
<p>On March 12th, 2014, 12:13 p.m. UTC, <b>Alex Merry</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;">Non-void slots are only useful if they work as a slot (ie: have some sort of side-effect) and might be called directly or with QMetaObject::invokeMethod(). If the method is const (like a getter), there's no point having it a slot at all; if you want to be able to use it with invokeMethod, you can just make it Q_INVOKABLE.</pre>
</blockquote>
<p>On March 12th, 2014, 1:25 p.m. UTC, <b>Aleix Pol Gonzalez</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;">Well, having const methods as slots doesn't make sense indeed, if it's not for exposing on the meta object system, that's why I said you could do a fast lxr. I just did, didn't find anything relevant.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thank you for looking it up :-)</pre>
<br />
<p>- David</p>
<br />
<p>On March 11th, 2014, 10:32 p.m. UTC, David Gil Oliva wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By David Gil Oliva.</div>
<p style="color: grey;"><i>Updated March 11, 2014, 10:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcompletion
</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;">Clean up KCompletionBox
-canceled() -> cancelled (private method)
-Deprecate sizeAndPosition() --> resizeAndReposition()
-Remove old comments and commented-out code
-Move some slots to be normal methods, since they don't seem to be able to
work as slots.</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;">It builds and tests pass.</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>src/kcompletionbox.h <span style="color: grey">(09b7527)</span></li>
<li>src/kcompletionbox.cpp <span style="color: grey">(92e87b3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116747/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>