[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 108992: Simple optimizations in SignalPlotter
From: "Raul Fernandes" <rgfernandes () gmail ! com>
Date: 2013-02-22 19:16:00
Message-ID: 20130222191600.18942.64270 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 230
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line230>
> >
> > no point in taking PODs out of the loop
Actually, the benefit exists in PODs too (the compiler has to adjust the stack each \
iteration) but is minor. I agree. I don't agree that it uglifies the code or make it \
less readable. To me, it doesn't make any difference to readability and has the \
(little) gain in performance. My opinion.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 781-782
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line781>
> >
> > this is unlikely to have any impact; PODs are cheap and compilers are smart.
I agree that the impact is minor, but if we educate the coders to always declare \
variables outside the loops the code will always be better and faster.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 784
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line784>
> >
> > const QList<double> &?
Good point. I didn't see that.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 1091-1092
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line1091>
> >
> > why not just avoid the assignment (and the hard to read global instantiation) \
> > with:
> > val = QString("%1 %2").arg(KGlobal::locale()->formatNumber(value, d->precision), \
> > d->unit);
> > i mean, if we're going to ugly it up, let's ugly it up as efficiently as we can \
> > ;)
Putting KGlobal::locale... in a new line will make more readable.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 1075
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line1075>
> >
> > this can probably go (see next comment)
Good point.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 1100
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line1100>
> >
> > unlikely to make any difference other than make the code harder to read.
Again, the benefit is minimal but educate the coders to always declare variables \
outside loops. Really, I don't see any ugliness. Maybe because I used to code this \
way.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 231-244
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line231>
> >
> > er .. this does nothing.
> >
> > the data member in the foreach loop is a *copy* of the QList<double> from \
> > d->plotData. assigning to it just assigns to that copy which only has scope \
> > within the foreach.
> > this *ought* to be:
> >
> > QMutableListIterator<QList<double> > it(d->plotData);
> > while (it.hasNext()) {
> > it.next();
> > newPlot.clear();
> > newPlot.append(data.at(newIndex));
> > it.setValue(newPlot);
> > }
> >
> >
Sorry but you are wrong in one point.
Creating the classe inside the loop leads the compiler to create the object, make the \
copy and destroy the object in each iteration. With my patch, it only makes the copy.
I have saw the constructors and destructors in profiler. After the patch, they \
disappear. I think there are something wrong with the code too.
The data variable will only be a copy and the statement:
data = newPlot;
will not change the actual list.
I think the code should be (using your suggestion):
QList<double> newPlot;
QList<double> data;
int newIndex;
QMutableListIterator<QList<double> > it(d->plotData);
while (it.hasNext()) {
data = it.next();
if (newOrder.count() != data.count()) {
kDebug() << "Serious problem in move sample. plotdata[i] has "
<< data.count() << " and neworder has " << newOrder.count();
} else {
newPlot.clear();
newPlot.reserve(newOrder.count());
for (int i = 0; i < newOrder.count(); i++) {
newIndex = newOrder[i];
newPlot.append(data.at(newIndex));
}
it.setValue(newPlot);
}
}
Maybe creating data as const reference (const QList<double> data = it.next()) will \
avoid the copy and the constructor and destructor. I will test it.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 860-863
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line860>
> >
> > this is exceedingly ugly and the PODs should not be incurring much overhead. \
> > keeping QList instantiation out of the loops makes sense, but not PODs.
The gain is minimal but I think it is valid to educate the coders to always declare \
variables outside the loops. It is a good practice. Do you agree?
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 785
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line785>
> >
> > since we're changing things here, how about the rather more readable:
> >
> > QListIterator<double> it(newestData);
> > it.toBack();
> > while (it.hasPrevious()) {
> > it.previous();
It can't be done as is.
The variable i will be used latter (d->plotColors[i].darkColor), so we have to use \
it.
> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 231
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line231>
> >
> > QList<double> &data
The idea is good but the problem is the data will be changed latter in the code.
- Raul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108992/#review27877
-----------------------------------------------------------
On Feb. 17, 2013, 12:57 p.m., Raul Fernandes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108992/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2013, 12:57 p.m.)
>
>
> Review request for Plasma.
>
>
> Description
> -------
>
> - create variables and classes outside the loops
> - reserve space in QList if we know already how many items will be added (avoid \
> unnecessary reallocations)
> - use const_iterator when possible
> - remove a useless call (p->setPen(Qt::NoPen) - it will be set latter before be \
> used)
> - avoid multiplications (x3, x2, x1 and x0)
>
>
> Diffs
> -----
>
> plasma/widgets/signalplotter.cpp 8e9e294
>
> Diff: http://git.reviewboard.kde.org/r/108992/diff/
>
>
> Testing
> -------
>
> I have tested with KDE 4.10 with no problems.
> I have seen a improvement of about 5% in drawPlots() function, the most expensive \
> function in painting.
>
> Thanks,
>
> Raul Fernandes
>
>
[Attachment #5 (text/html)]
<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/108992/">http://git.reviewboard.kde.org/r/108992/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line230" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::reorderPlots(const QList<uint>& newOrder)</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">230</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">int</span> <span class="n">newIndex</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;">no point in taking PODs \
out of the loop</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;">Actually, \
the benefit exists in PODs too (the compiler has to adjust the stack each iteration) \
but is minor. I agree. I don't agree that it uglifies the code or make it less \
readable. To me, it doesn't make any difference to readability and has the \
(little) gain in performance. My opinion. </pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line231" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::reorderPlots(const QList<uint>& newOrder)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">229</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">foreach</span> <span class="p">(</span><span class="n">QList</span><span \
class="o"><</span><span class="kt">double</span><span class="o">></span> <span \
class="n">data</span><span class="p">,</span> <span class="n">d</span><span \
class="o">-></span><span class="n">plotData</span><span class="p">)</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">231</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span> <span \
class="p">(</span><span class="n">QList</span><span class="o"><</span><span \
class="kt">double</span><span class="o">></span> <span class="n">data</span><span \
class="p">,</span> <span class="n">d</span><span class="o">-></span><span \
class="n">plotData</span><span class="p">)</span> <span class="p">{</span></pre></td> \
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">230</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="n">newOrder</span><span \
class="p">.</span><span class="n">count</span><span class="p">()</span> <span \
class="o">!=</span> <span class="n">data</span><span class="p">.</span><span \
class="n">count</span><span class="p">())</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">232</font></th> <td bgcolor="#ffffff" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="n">newOrder</span><span \
class="p">.</span><span class="n">count</span><span class="p">()</span> <span \
class="o">!=</span> <span class="n">data</span><span class="p">.</span><span \
class="n">count</span><span class="p">())</span> <span class="p">{</span></pre></td> \
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">231</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">kDebug</span><span class="p">()</span> <span class="o"><<</span> \
<span class="s">"Serious problem in move sample. plotdata[i] has \
"</span></pre></td> <th bgcolor="#f0f0f0" style="border-left: 1px solid \
#C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">233</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">kDebug</span><span \
class="p">()</span> <span class="o"><<</span> <span class="s">"Serious \
problem in move sample. plotdata[i] has "</span></pre></td> </tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">232</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="o"><<</span> <span class="n">data</span><span class="p">.</span><span \
class="n">count</span><span class="p">()</span> <span class="o"><<</span> <span \
class="s">" and neworder has "</span> <span class="o"><<</span> <span \
class="n">newOrder</span><span class="p">.</span><span class="n">count</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">234</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span \
class="o"><<</span> <span class="n">data</span><span class="p">.</span><span \
class="n">count</span><span class="p">()</span> <span class="o"><<</span> <span \
class="s">" and neworder has "</span> <span class="o"><<</span> <span \
class="n">newOrder</span><span class="p">.</span><span class="n">count</span><span \
class="p">();</span></pre></td> </tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">233</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="p">}</span> <span class="k">else</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">235</font></th> <td bgcolor="#ffffff" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="p">}</span> <span class="k">else</span> <span class="p">{</span></pre></td> \
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">234</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n"><span class="hl">QList</span></span><span class="o"><span \
class="hl"><</span></span><span class="kt"><span \
class="hl">double</span></span><span class="o"><span \
class="hl">></span></span><span class="hl"> </span><span \
class="n">newPlot</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">236</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">newPlot</span><span class="p"><span class="hl">.</span></span><span \
class="n"><span class="hl">clear</span></span><span class="p"><span \
class="hl">()</span>;</span></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">237</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="n">newPlot</span><span class="p">.</span><span \
class="n">reserve</span><span class="p">(</span><span class="n">newOrder</span><span \
class="p">.</span><span class="n">count</span><span class="p">());</span></pre></td> \
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">235</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span> <span class="p">(</span><span class="kt">int</span> <span \
class="n">i</span> <span class="o">=</span> <span class="mi">0</span><span \
class="p">;</span> <span class="n">i</span> <span class="o"><</span> <span \
class="n">newOrder</span><span class="p">.</span><span class="n">count</span><span \
class="p">();</span> <span class="n">i</span><span class="o">++</span><span \
class="p">)</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">238</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span> <span class="p">(</span><span class="kt">int</span> <span \
class="n">i</span> <span class="o">=</span> <span class="mi">0</span><span \
class="p">;</span> <span class="n">i</span> <span class="o"><</span> <span \
class="n">newOrder</span><span class="p">.</span><span class="n">count</span><span \
class="p">();</span> <span class="n">i</span><span class="o">++</span><span \
class="p">)</span> <span class="p">{</span></pre></td> </tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">236</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="hl"> </span><span class="kt"><span class="hl">int</span></span> <span \
class="n">newIndex</span> <span class="o">=</span> <span \
class="n">newOrder</span><span class="p">[</span><span class="n">i</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">239</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">newIndex</span> \
<span class="o">=</span> <span class="n">newOrder</span><span class="p">[</span><span \
class="n">i</span><span class="p">];</span></pre></td> </tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">237</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">newPlot</span><span class="p">.</span><span class="n">append</span><span \
class="p">(</span><span class="n">data</span><span class="p">.</span><span \
class="n">at</span><span class="p">(</span><span class="n">newIndex</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">240</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span \
class="n">newPlot</span><span class="p">.</span><span class="n">append</span><span \
class="p">(</span><span class="n">data</span><span class="p">.</span><span \
class="n">at</span><span class="p">(</span><span class="n">newIndex</span><span \
class="p">));</span></pre></td> </tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">238</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">241</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td> \
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">239</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">data</span> <span class="o">=</span> <span class="n">newPlot</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">242</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">data</span> <span \
class="o">=</span> <span class="n">newPlot</span><span class="p">;</span></pre></td> \
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">240</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">243</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td> \
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">241</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">244</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <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;">er .. this does nothing.
the data member in the foreach loop is a *copy* of the QList<double> from \
d->plotData. assigning to it just assigns to that copy which only has scope within \
the foreach.
this *ought* to be:
QMutableListIterator<QList<double> > it(d->plotData);
while (it.hasNext()) {
it.next();
newPlot.clear();
newPlot.append(data.at(newIndex));
it.setValue(newPlot);
}
</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;">Sorry but \
you are wrong in one point. Creating the classe inside the loop leads the compiler to \
create the object, make the copy and destroy the object in each iteration. With my \
patch, it only makes the copy. I have saw the constructors and destructors in \
profiler. After the patch, they disappear. I think there are something wrong with the \
code too. The data variable will only be a copy and the statement:
data = newPlot;
will not change the actual list.
I think the code should be (using your suggestion):
QList<double> newPlot;
QList<double> data;
int newIndex;
QMutableListIterator<QList<double> > it(d->plotData);
while (it.hasNext()) {
data = it.next();
if (newOrder.count() != data.count()) {
kDebug() << "Serious problem in move sample. plotdata[i] has \
"
<< data.count() << " and neworder has " \
<< newOrder.count(); } else {
newPlot.clear();
newPlot.reserve(newOrder.count());
for (int i = 0; i < newOrder.count(); i++) {
newIndex = newOrder[i];
newPlot.append(data.at(newIndex));
}
it.setValue(newPlot);
}
}
Maybe creating data as const reference (const QList<double> data = it.next()) \
will avoid the copy and the constructor and destructor. I will test it.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line231" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::reorderPlots(const QList<uint>& newOrder)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">229</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">foreach</span> <span class="p">(</span><span class="n">QList</span><span \
class="o"><</span><span class="kt">double</span><span class="o">></span> <span \
class="n">data</span><span class="p">,</span> <span class="n">d</span><span \
class="o">-></span><span class="n">plotData</span><span class="p">)</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">231</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span> <span \
class="p">(</span><span class="n">QList</span><span class="o"><</span><span \
class="kt">double</span><span class="o">></span> <span class="n">data</span><span \
class="p">,</span> <span class="n">d</span><span class="o">-></span><span \
class="n">plotData</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;">QList<double> \
&data</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;">The idea is \
good but the problem is the data will be changed latter in the code.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line781" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawTopBarContents(QPainter *p, int x, int width, int \
height)</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">781</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">double</span> <span class="n">newest_datapoint</span><span \
class="p">;</span></pre></td> </tr>
<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">782</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">int</span> <span class="n">start</span><span class="p">,</span> <span \
class="n">end</span><span class="p">,</span> <span class="n">start2</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;">this is unlikely to have \
any impact; PODs are cheap and compilers are smart.</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;">I agree \
that the impact is minor, but if we educate the coders to always declare variables \
outside the loops the code will always be better and faster.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line784" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawTopBarContents(QPainter *p, int x, int width, int \
height)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">777</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">QList</span><span class="o"><</span><span class="kt">double</span><span \
class="o">></span> <span class="n">newestData</span> <span class="o">=</span> \
<span class="n">d</span><span class="o">-></span><span \
class="n">plotData</span><span class="p">.</span><span class="n">first</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">784</font></th> <td bgcolor="#ffffff" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">QList</span><span \
class="o"><</span><span class="kt">double</span><span class="o">></span> <span \
class="n">newestData</span> <span class="o">=</span> <span class="n">d</span><span \
class="o">-></span><span class="n">plotData</span><span class="p">.</span><span \
class="n">first</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;">const \
QList<double> &?</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;">Good point. \
I didn't see that.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line785" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawTopBarContents(QPainter *p, int x, int width, int \
height)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">778</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span> <span class="p">(</span><span class="kt">int</span> <span \
class="n">i</span> <span class="o">=</span> <span class="n">newestData</span><span \
class="p">.</span><span class="n">count</span><span class="p">()</span><span \
class="o">-</span><span class="mi">1</span><span class="p">;</span> <span \
class="n">i</span> <span class="o">>=</span> <span class="mi">0</span><span \
class="p">;</span> <span class="o">--</span><span class="n">i</span><span \
class="p">)</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">785</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span> <span class="p">(</span><span class="kt">int</span> <span \
class="n">i</span> <span class="o">=</span> <span class="n">newestData</span><span \
class="p">.</span><span class="n">count</span><span class="p">()</span><span \
class="o">-</span><span class="mi">1</span><span class="p">;</span> <span \
class="n">i</span> <span class="o">>=</span> <span class="mi">0</span><span \
class="p">;</span> <span class="o">--</span><span class="n">i</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;">since we're changing \
things here, how about the rather more readable:
QListIterator<double> it(newestData);
it.toBack();
while (it.hasPrevious()) {
it.previous();</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;">It \
can't be done as is. The variable i will be used latter \
(d->plotColors[i].darkColor), so we have to use it.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line860" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawPlots(QPainter *p, int top, int w, int h, int \
horizontalScale)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">852</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">859</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">float</span> <span class="n">x0</span><span class="p">,</span> <span \
class="n">x1</span><span class="p">,</span> <span class="n">x2</span><span \
class="p">,</span> <span class="n">x3</span><span class="p">,</span> <span \
class="n">y0</span><span class="p">,</span> <span class="n">y1</span><span \
class="p">,</span> <span class="n">y2</span><span class="p">,</span> <span \
class="n">y3</span><span class="p">;</span></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">860</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">int</span> <span class="n">offset</span><span \
class="p">;</span></pre></td> </tr>
<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">861</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">double</span> <span class="n">max_y</span><span class="p">,</span> <span \
class="n">min_y</span><span class="p">,</span> <span \
class="n">current_maxvalue</span><span class="p">,</span> <span \
class="n">current_minvalue</span><span class="p">;</span></pre></td> </tr>
<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">862</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">float</span> <span class="n">delta_y0</span><span class="p">,</span> <span \
class="n">delta_y1</span><span class="p">,</span> <span \
class="n">delta_y2</span><span class="p">,</span> <span \
class="n">delta_y3</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;">this is exceedingly ugly \
and the PODs should not be incurring much overhead. keeping QList instantiation out \
of the loops makes sense, but not PODs.</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;">The gain is \
minimal but I think it is valid to educate the coders to always declare variables \
outside the loops. It is a good practice. Do you agree?</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line1075" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawAxisText(QPainter *p, int top, int h)</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">1063</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">QString</span> <span class="n">number</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;">this can probably go \
(see next comment)</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;">Good \
point.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line1091" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawAxisText(QPainter *p, int top, int h)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">1071</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">QString</span></span> <span \
class="n">number</span> <span class="o">=</span> <span class="n">KGlobal</span><span \
class="o">::</span><span class="n">locale</span><span class="p">()</span><span \
class="o">-></span><span class="n">formatNumber</span><span \
class="p">(</span><span class="n">value</span><span class="p">,</span> <span \
class="n">d</span><span class="o">-></span><span class="n">precision</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">1078</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">number</span> <span \
class="o">=</span> <span class="n">KGlobal</span><span class="o">::</span><span \
class="n">locale</span><span class="p">()</span><span class="o">-></span><span \
class="n">formatNumber</span><span class="p">(</span><span \
class="n">value</span><span class="p">,</span> <span class="n">d</span><span \
class="o">-></span><span class="n">precision</span><span \
class="p">);</span></pre></td> </tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">1072</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">val</span> <span class="o">=</span> <span class="n">QString</span><span \
class="p">(</span><span class="s">"%1 %2"</span><span \
class="p">).</span><span class="n">arg</span><span class="p">(</span><span \
class="n">number</span><span class="p">,</span> <span class="n">d</span><span \
class="o">-></span><span class="n">unit</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">1079</font></th> <td bgcolor="#ffffff" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">val</span> <span class="o">=</span> <span class="n">QString</span><span \
class="p">(</span><span class="s">"%1 %2"</span><span \
class="p">).</span><span class="n">arg</span><span class="p">(</span><span \
class="n">number</span><span class="p">,</span> <span class="n">d</span><span \
class="o">-></span><span class="n">unit</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;">why not just avoid the \
assignment (and the hard to read global instantiation) with:
val = QString("%1 %2").arg(KGlobal::locale()->formatNumber(value, \
d->precision), d->unit);
i mean, if we're going to ugly it up, let's ugly it up as efficiently as we \
can ;)</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;">Putting \
KGlobal::locale... in a new line will make more readable.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 22nd, 2013, 1:32 a.m. UTC, <b>Aaron J. \
Seigo</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/108992/diff/1/?file=114209#file114209line1100" \
style="color: black; font-weight: bold; text-decoration: \
underline;">plasma/widgets/signalplotter.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 \
SignalPlotter::drawHorizontalLines(QPainter *p, int top, int w, int h)</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">1087</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">int</span> <span class="n">y_coord</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;">unlikely to make any \
difference other than make the code harder to read.</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;">Again, the \
benefit is minimal but educate the coders to always declare variables outside loops. \
Really, I don't see any ugliness. Maybe because I used to code this way.</pre> \
<br />
<p>- Raul</p>
<br />
<p>On February 17th, 2013, 12:57 p.m. UTC, Raul Fernandes 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 Plasma.</div>
<div>By Raul Fernandes.</div>
<p style="color: grey;"><i>Updated Feb. 17, 2013, 12:57 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;">- create variables and classes outside the loops
- reserve space in QList if we know already how many items will be added (avoid \
unnecessary reallocations)
- use const_iterator when possible
- remove a useless call (p->setPen(Qt::NoPen) - it will be set latter before be \
used)
- avoid multiplications (x3, x2, x1 and x0)</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;">I have tested with KDE 4.10 with no problems. I have seen a improvement \
of about 5% in drawPlots() function, the most expensive function in painting.</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>plasma/widgets/signalplotter.cpp <span style="color: grey">(8e9e294)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108992/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic