Review kdelibs whiting/fixQByteArrays

[Olivier Goffart]

Le Saturday 30 April 2011, David Faure a écrit :
> On Friday 29 April 2011, Jeremy Whiting wrote:
> > Since David added QT_USE_FAST_OPERATOR_PLUS to our build rules for
> > kdelibs, it wont build here.
> 
> Oops?
> 
> > I've got a commit on the whiting/fixQByteArrays that
> > fixes this, but I'd like someone to review it before I merge to master.
> > Anyone got a moment to test and review it?

Where is the patch?

> 
> Reviewboard is a better tool for this, but okay, I'm not in a position to
> complain, as the guy with the hat of shame for breaking the build :)
> 
> -    QString testpath = "file://" + path + "/kdelibs/khtml/test/";
> +    QString testpath = QString("file://" + path + "/kdelibs/khtml/test/");
> Can be done simpler as
>    QString testpath("file://" + path + "/kdelibs/khtml/test/");
> 
> Surprisingly (or rather unsurprisingly, since I did test before
> committing), it compiles without this change. gcc-4.4.5 here, kubuntu;
> which compiler do you use?
> 
> This is a case of an explicit conversion to QString so in theory it should
> just work (unlike all the template cases I fixed in my commit), but maybe
> it breaks on some compilers because the first arg is a char litteral.

This is because of the changes to QStringBuilder which now also works with 
QByteArray in Qt4.8,  meaning this case does not convert to QString, but to 
QByteArray

> I guess a more logical change, then, could be
>  QString testpath = QString("file://") + path + "...";

The best in this case is
QString testpath = QLatin1String("file://") + path + 
QLatin1String("/kdelibs/khtml/test/");
That way you acheive the best performence of only one allocation and one copy.
(one allocation, one copy of the string)
(If you use at least one QString or QLatin1String, it will convert everything 
tp QString, and not QByteArray) 


> The other changes are related to QByteArray, which leaves me completely
> stumped. Fast operator plus is for QString only. How can m_tokens[id+uuid]
> break, when id and uuid are bytearrays? Oh, m_tokens takes a QString as
> key, blah.
> Please test and apply the real fix:
> - QMap<QString, QByteArray>         m_tokens;
> + QMap<QByteArray, QByteArray>         m_tokens;
> AFAICS the key is used as a bytearray everywhere in
> kdelibs/plasnma/private/serviceprovider.cpp

Yes, now QStringBuider work with QByteArray as well in Qt 4.8

> For the last change:
> -        removeDir(QLatin1String("packageRoot/" + dir.toLatin1() +
> "/contents/code"));
> +        removeDir(QLatin1String(QByteArray("packageRoot/" + dir.toLatin1()
> + "/contents/code")));
> 
> I think it would be simpler to write
>  removeDir(QLatin1String("packageRoot/") + dir + "/contents/code"));
> (or QString::fromLatin1 instead of QLatin1String, if necessary)
> (and maybe with a QLatin1String around the last arg, if this code uses "no
> cast from ascii". The compiler will tell you ;)  ).

Yes, david is right: use QLatin1String around string litteral that are meant 
to be converted to QString.