----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108992/#review27877 -----------------------------------------------------------
ints and other POD types do not need to be pulled from the loops. it just uglifies with no benefit. moreover, there is a bug that exists in the code which has gone unfixed (noted below) plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20901> no point in taking PODs out of the loop plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20892> QList<double> &data plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20893> 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); } plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20894> this is unlikely to have any impact; PODs are cheap and compilers are smart. plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20895> const QList<double> &? plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20900> since we're changing things here, how about the rather more readable: QListIterator<double> it(newestData); it.toBack(); while (it.hasPrevious()) { it.previous(); plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20896> 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. plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20899> this can probably go (see next comment) plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20898> 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 ;) plasma/widgets/signalplotter.cpp <http://git.reviewboard.kde.org/r/108992/#comment20897> unlikely to make any difference other than make the code harder to read. - Aaron J. Seigo 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
