Review Request: Allow kdelibs compilation with QT_USE_FAST_CONCATENATION & QT_USE_FAST_OPERATOR_PLUS flags...

[Olivier Goffart]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101108/108e8205/attachment.htm>