> On 2010-11-08 07:55:39, Stephen Kelly wrote: > > trunk/KDE/kdelibs/kate/plugins/exporter/htmlexporter.cpp, line 102 > > <http://svn.reviewboard.kde.org/r/5795/diff/1/?file=40773#file40773line102> > > > > 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. > > Dawit Alemayehu wrote: > 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. > > Stephen Kelly wrote: > > > 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.
Looking at the code generated by gcc in -O2, QLatin1Litteral and QLatin1String expand the exact same code in almost all cases. (In most cases, the call to strlen is inline, and gcc has a builtin for that) QLatin1Litteral requires to include <QStringBuilder>, which cannot be include by default on all platform due to a bug in some compilers. - Olivier ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5795/#review8548 ----------------------------------------------------------- On 2010-11-08 17:12:54, Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/5795/ > ----------------------------------------------------------- > > (Updated 2010-11-08 17:12:54) > > > Review request for kdelibs. > > > Summary > ------- > > 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. > > > Diffs > ----- > > trunk/KDE/kdelibs/kate/plugins/exporter/htmlexporter.cpp 1194086 > trunk/KDE/kdelibs/kdecore/kconfig_compiler/kconfig_compiler.cpp 1194086 > trunk/KDE/kdelibs/kdewebkit/kwebpluginfactory.cpp 1194086 > trunk/KDE/kdelibs/khtml/ecma/kjs_scriptable.cpp 1194086 > trunk/KDE/kdelibs/khtml/ecma/kjs_scriptable.cpp 1194086 > trunk/KDE/kdelibs/knewstuff/knewstuff3/core/cache.cpp 1194086 > trunk/KDE/kdelibs/plasma/datacontainer.cpp 1194086 > trunk/KDE/kdelibs/plasma/widgets/declarativewidget.cpp 1194086 > > Diff: http://svn.reviewboard.kde.org/r/5795/diff > > > Testing > ------- > > > Thanks, > > Dawit > >
