<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://git.reviewboard.kde.org/r/109524/">http://git.reviewboard.kde.org/r/109524/</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 16th, 2013, 7:22 p.m. UTC, <b>Stephen Kelly</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="http://git.reviewboard.kde.org/r/109524/diff/1/?file=120105#file120105line135" style="color: black; font-weight: bold; text-decoration: underline;">tier1/kcodecs/autotests/kcharsetstest.cpp</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; ">void KCharsetsTest::testFromEntity()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">135</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">KCharsets</span></span><span class="hl"> </span><span class="o"><span class="hl">*</span></span><span class="n"><span class="hl">singleton</span></span><span class="hl"> </span><span class="o"><span class="hl">=</span></span> <span class="n">KCharsets</span><span class="o">::</span><span class="n">charsets</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">135</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KCharsets</span><span class="o">::</span><span class="n">charsets</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;">Warnings are sometimes valid and create a todo list. Changes like this look like just hiding the warning for the sake of it. It looks like 'done' for this is something different, not just removing the variable.
Please double check that the other warnings you are resolving should really be resolved in a naive way.</pre>
</blockquote>
<p>On March 17th, 2013, 9:35 p.m. UTC, <b>Miquel Canes 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;">I think that this variable is a copy&paste from the other tests of this class. In this test we also have a FIX comment. Why don't remove the warning?
</pre>
</blockquote>
<p>On March 18th, 2013, 2:36 p.m. UTC, <b>Stephen Kelly</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;">Because in 'fixing the warning', you also remove the compiler-generated TODO list item to implement the correct fix.
Does changing this line make anything better, apart from making the compiler not warn? What is the value of invoking KCharsets::charsets(); in the test and not doing anything else?</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;">What do you think about adding a message like this and remove the test "unused" code?
#pragma message("KCharsets::toEntity test not implemented.")</pre>
<br />
<p>- Miquel</p>
<br />
<p>On April 7th, 2013, 10:26 p.m. UTC, Miquel Canes Gonzalez wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 and Aleix Pol Gonzalez.</div>
<div>By Miquel Canes Gonzalez.</div>
<p style="color: grey;"><i>Updated April 7, 2013, 10:26 p.m.</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;">Remove some compile warnings like unused variables, reordering initializers, uint -> int...</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;">compile again with less warnings.</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>interfaces/kimproxy/library/kimproxy.cpp <span style="color: grey">(66e263f)</span></li>
<li>kdecore/io/kdebug.cpp <span style="color: grey">(6fd7584)</span></li>
<li>kdeui/dialogs/kedittoolbar.cpp <span style="color: grey">(b03d2cb)</span></li>
<li>kdeui/dialogs/kinputdialog.cpp <span style="color: grey">(28ce074)</span></li>
<li>kdeui/dialogs/kshortcutseditordelegate.cpp <span style="color: grey">(d458106)</span></li>
<li>kdeui/kernel/kstyle.cpp <span style="color: grey">(681b940)</span></li>
<li>kdeui/tests/kreplacetest.cpp <span style="color: grey">(c59bd76)</span></li>
<li>kdeui/util/kmodifierkeyinfoprovider_dummy.cpp <span style="color: grey">(d456e82)</span></li>
<li>kdeui/widgets/kdatetable.cpp <span style="color: grey">(4d24290)</span></li>
<li>kdeui/widgets/kkeysequencewidget.cpp <span style="color: grey">(28eb47e)</span></li>
<li>kio/bookmarks/kbookmarkimporter_opera.cc <span style="color: grey">(d39f7a4)</span></li>
<li>kio/kio/ksambashare.cpp <span style="color: grey">(0239af5)</span></li>
<li>kio/tests/kbookmarktest.cpp <span style="color: grey">(3a0c742)</span></li>
<li>kioslave/file/file.cpp <span style="color: grey">(d57517b)</span></li>
<li>knewstuff/knewstuff3/ui/itemsgridviewdelegate.cpp <span style="color: grey">(eb72240)</span></li>
<li>staging/ki18n/src/klocalizedstring.cpp <span style="color: grey">(452dd59)</span></li>
<li>staging/kwidgets/src/icons/kiconengine.cpp <span style="color: grey">(3e5fcc0)</span></li>
<li>tier1/itemmodels/src/kdescendantsproxymodel.cpp <span style="color: grey">(8ee0702)</span></li>
<li>tier1/itemmodels/src/kselectionproxymodel.cpp <span style="color: grey">(f002f44)</span></li>
<li>tier1/kcodecs/autotests/kcharsetstest.cpp <span style="color: grey">(0c7aac1)</span></li>
<li>tier1/kjs/src/kjs/array_object.cpp <span style="color: grey">(b726c09)</span></li>
<li>tier1/kjs/src/kjs/bytecode/machine.cpp.in <span style="color: grey">(a434c36)</span></li>
<li>tier1/kwindowsystem/src/kxutils.cpp <span style="color: grey">(f3e276a)</span></li>
<li>tier1/solid/src/solid/backends/upnp/upnpinternetgateway.cpp <span style="color: grey">(2c4ca85)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109524/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>