<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://svn.reviewboard.kde.org/r/5795/">http://svn.reviewboard.kde.org/r/5795/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 8th, 2010, 7:55 a.m., <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://svn.reviewboard.kde.org/r/5795/diff/1/?file=40773#file40773line102" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kate/plugins/exporter/htmlexporter.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 HTMLExporter::exportText(const QString& text, const KTextEditor::Attribute::Ptr& attrib)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">102</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">.</span><span class="n">arg</span><span class="p">(</span><span class="n">writeForeground</span> <span class="o">?</span> <span class="s">"color:"</span> <span class="o">+</span> <span class="n">attrib</span><span class="o">-></span><span class="n">foreground</span><span class="p">().</span><span class="n">color</span><span class="p">().</span><span class="n">name</span><span class="p">()</span> <span class="o">+</span> <span class="sc">';'</span> <span class="o">:</span> <span class="s">""</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">102</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">.</span><span class="n">arg</span><span class="p">(</span><span class="n">writeForeground</span> <span class="o">?</span> <span class="n"><span class="hl">QString</span></span><span class="p"><span class="hl">(</span></span><span class="s">"color:"</span> <span class="o">+</span> <span class="n">attrib</span><span class="o">-></span><span class="n">foreground</span><span class="p">().</span><span class="n">color</span><span class="p">().</span><span class="n">name</span><span class="p">()</span> <span class="o">+</span> <span class="sc">';'</span><span class="p"><span class="hl">)</span></span> <span class="o">:</span> <span class="s">""</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;">Shouldn't this be something like
? QLatin1Literal("color:") + attrib->foreground().color().name() + QLatin1Char(';') : QString() )
In general in this patch I think you should be using QLatin1Literal instead of QLatin1String in most places.</pre>
</blockquote>
<p>On November 8th, 2010, 5:10 p.m., <b>Dawit Alemayehu</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;">Yes, I probably should use have surrounded the literal text with QLatin1String. However, using QLatin1Literal is outside the scope of this patch because it is a further optimization work. Literally there are thousands of such changes that need to be made in kdelibs and all those changes need to be done in one go together. More over there are lots of places in kdelibs where strings are concatenated using QString::arg, which is the slowest possible way of accomplishing such tasks. Those also need to be changed. The point being all the optimization work should happen in another set of patches.
Actually, I have no idea why setting these two these two flags does not automatically result in the conversion of QLatin1String calls to QLatin1Literal ones. That would make the job much simpler. Anyhow, it is outside the scope of this patch to do those changes. This patch is only attempting to make sure turning on those flags lets you compile kdelibs.</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;">
> Yes, I probably should use have surrounded the literal text with
> QLatin1String. However, using QLatin1Literal is outside the scope of this
> patch because it is a further optimization work. Literally there are
> thousands of such changes that need to be made in kdelibs and all those
> changes need to be done in one go together.
I don't see why it's important that all introduction of QLatin1Literal is done in one go.
> More over there are lots of
> places in kdelibs where strings are concatenated using QString::arg, which
> is the slowest possible way of accomplishing such tasks. Those also need
> to be changed. The point being all the optimization work should happen in
> another set of patches.
>
> Actually, I have no idea why setting these two these two flags does not
> automatically result in the conversion of QLatin1String calls to
> QLatin1Literal ones.
It's not source compatible. QLatin1String has operator QString(), but QLatin1Literal doesn't. There's a comment in the code to merge the two, but that wouldn't be binary compatible, so that's a Qt5 thing.
> That would make the job much simpler. Anyhow, it is
> outside the scope of this patch to do those changes. This patch is only
> attempting to make sure turning on those flags lets you compile kdelibs.
Understood. In that case, ship it.</pre>
<br />
<p>- Stephen</p>
<br />
<p>On November 8th, 2010, 5:12 p.m., Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.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 Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated 2010-11-08 17:12:54</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 attached patch makes it possible to compile kdelibs using the global Qt defines that improve the performance of QString concatenation, i.e. QT_USE_FAST_CONCATENATION and QT_USE_FAST_OPERATOR. For more details about faster QString concatenation, see "More Efficient String Construction" section of QString's documentation.</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/kate/plugins/exporter/htmlexporter.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/kdecore/kconfig_compiler/kconfig_compiler.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/kdewebkit/kwebpluginfactory.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/khtml/ecma/kjs_scriptable.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/khtml/ecma/kjs_scriptable.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/knewstuff/knewstuff3/core/cache.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/plasma/datacontainer.cpp <span style="color: grey">(1194086)</span></li>
<li>trunk/KDE/kdelibs/plasma/widgets/declarativewidget.cpp <span style="color: grey">(1194086)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5795/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>