D25039: Fix Clazy performance issues, const &, noexcept

[Friedrich W. H. Kossebau]

kossebau added a comment.


  As excuse for bad drive-by comment, here now hopefully making up a bit by giving some in-detail review, please see in-line comments.
  
  No idea about exceptions.
  
  I would the noexcept also make a different commit, for more separation of concerns. When looking at the history, such "All kind of tool-found improvements" are not nice when checking for changes which could have added regressions. IMHO.

INLINE COMMENTS

> kcoredirlister.cpp:1551
>          DirItem *dir = itu.value();
> -        QUrl oldDirUrl(itu.key());
> +        const QUrl &oldDirUrl(itu.key());
>          qCDebug(KIO_CORE_DIRLISTER) << "itemInUse:" << oldDirUrl;

For consistency I would make this an assignment, IMHO also easier to read and not to be mixed up with a function call on quick sight (quick sights might be a problem of mine, but surely also others :) ):

  onst QUrl &oldDirUrl = itu.key();

> kcoredirlister.cpp:1944
>      for (; itu != ituend; ++itu) {
> -        const QUrl deletedUrl(itu.key());
> +        const QUrl &deletedUrl(itu.key());
>          if (dirUrl == deletedUrl || dirUrl.isParentOf(deletedUrl)) {

dito

> slavebase.h:363
>       */
> -    QString configValue(QString key, const QString &defaultValue=QString()) const;
> +    QString configValue(const QString &key, const QString &defaultValue=QString()) const;
>  

while touching the line, please also add spaces around the = -> `defaultValue = QString())`

> ftp.cpp:1378
>      Q_ASSERT(!filename.isEmpty());
> -    QString search = filename;
> +    const QString &search = filename;
>  

This here makes the code fragile and more confusing. What is the difference between `filename` & `search`? Why is `filename` not const? Can it ever change? Would that be fine for `search` to change as well?
So while seemingly a correct optimization, as `filename` seems not passed to any method modifying it, the resulting code is very strange to a human reader, the intent behind to have `search` being a const reference to `filename` seems mysterious.

Without having understood the code, I would simply also make `filename` a const variable, and add a hint why search is a const reference only (hinting this is for optimization).
Though actually `search` could be possibly be removed and checked what the purpose of the asserts have been and merge this with the code upfront. But outside of scope here, so leaving just a TODO for the next person should be fine. One can still be the next person oneself :)

> kcookiejar.cpp:1103
>  {
> -    QString domain(_domain);
> +    const QString &domain(_domain);
>      KHttpCookieList *cookieList = m_cookieDomains.value(domain);

This here seems some overleft from when the method actually needed a modfyable copy of _domain. Seems this in no longer the case.
So we can just rename `_domain` to `domain` in the arg list instead, and be done.

> script.cpp:201
>  // @returns true if @p host doesn't contains a domain part
> -Q_INVOKABLE QJSValue IsPlainHostName(QString string)
> +Q_INVOKABLE QJSValue IsPlainHostName(const QString &string)
>  {

Not an expert on Q_INVOKABLE & script access, I have seen people using non-const-ref arguments, perhaps they are needed other than with signals & slots?

Asked the person who wrote this code in D23801#556957 <https://phabricator.kde.org/D23801#556957>

> sslui.cpp:87
>                  QList<KSslError::Error> errors;
> -                for (const KSslError &error : qAsConst(ud->sslErrors)) {
> +                for (const QSslError &error : qAsConst(ud->sslErrors)) {
>                      if (error.certificate() == cert) {

Seems this slipped in, being out of scope :) So please remove from patch again.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25039

To: meven, #frameworks, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191031/ad5b76e1/attachment-0001.html>