Review kdelibs whiting/fixQByteArrays

[Jeremy Whiting]

On Apr 29, 2011 6:26 PM, "David Faure" <faure at kde.org> wrote:
>
> 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?
>
> 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 :)
>

It could be because arch recently switched to gcc 4.6. Could also be because
I'm trying to use qt 4.8 from git. I'll take a try at your suggestions when
I get up in the morning.

Jeremy

> -    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.
>
> I guess a more logical change, then, could be
>  QString testpath = QString("file://") + path + "...";
>
> 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
>
> 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 ;)  ).
>
> --
> David Faure, faure at kde.org, http://www.davidfaure.fr
> Sponsored by Nokia to work on KDE, incl. Konqueror (
http://www.konqueror.org).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110429/f8ff388f/attachment.htm>