> 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.
> 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. - Stephen ----------------------------------------------------------- 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 > >
