D25039: Fix Clazy performance issues, const

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:f2a3a78972b2: Fix Clazy performance issues, const  
(authored by meven).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69178=69179

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69178.
meven added a comment.


  Rebasing on master

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69121=69178

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D25039#557609 , @meven wrote:
  
  > Feel free to do the code removal,
  
  
  Done now, as I had the files still open.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25039

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

To: meven, #frameworks, dfaure, kossebau
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven added a comment.


  In D25039#557579 , @kossebau wrote:
  
  > Not tested, only read code. Looks good to me.
  >  Please remove the newInstance method in a direct commit before, and drop 
change from this patch. (If you prefer, can do the remove commit as well myself)
  
  
  I did test it. Maybe not with the exact last changeset. I will run the tests 
again before pushing juste to make sure.
  Feel free to do the code removal, if I don't do it first, I will be afk for a 
few days, I will rebase on master the branch.
  Btw I have already commited the changes to SlaveBase::configValue si it makes 
it to KF5.64 regardless of when this vers merged.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25039

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

To: meven, #frameworks, dfaure, kossebau
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Friedrich W. H. Kossebau]

kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.


  Not tested, only read code. Looks good to me.
  Please remove the newInstance method in a direct commit before, and drop 
change from this patch. (If you prefer, can do the remove commit as well myself)

INLINE COMMENTS

> kcookieserver.h:106
>  private:
> -virtual int newInstance(QList)
> +virtual int newInstance(const QList&)
>  {

As there is no override changed in this patch, I found this suspicious and had 
a look:
seems this is dead code once introduced in 2002, when KCookieServer had been 
made a DCOP object, and KUniqueApplication asked subclasses to implement a 
virtual "int newInstance();" method.
See https://phabricator.kde.org/R446:1276fdc2bdbf7fd0236b8630cc1e529e3a6c4fe5

Nothing is using this method anymore now, so it can instead be simply removed, 
in a separate commit.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25039

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

To: meven, #frameworks, dfaure, kossebau
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69121.
meven marked 7 inline comments as done.
meven added a comment.


  Review feedback

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69083=69121

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.h
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

To: meven, #frameworks, dfaure
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in ftp.cpp:1376
> IIUC, the compiler will use a temporary object to hold the return of 
> tempurl.fileName().
> 
> The temporary is used to initialize filename, and then it's gone:
> const QString filename = tempurl.filename();
> 
> filename here is a reference-to-const to the temporary object and the 
> temporary will have the same lifetime as the reference (for objects on the 
> stack), so until the end of the scope:
> const QString  = tempurl.fileName();
> c.f. 
> https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
> 
> In my mind it kind of makes more sense to use a reference-to-const when the 
> rvalue (tempurl.fileName()) is not a temporary object, because I am saving 
> nothing by using a reference here since the compiler will create the 
> temporary object and hold it until the end of the scope anyway.
> 
> In this particular case, it's probably exactly the same whether the temporary 
> is used to initialize a const non-reference object and then (the temporary) 
> is dropped/gone or the object being initialized is a reference to const to 
> the temporary until the end of the scope...
> 
> job->statResult() and job->url() are different because both of them return a 
> reference to const.

Thanks for the reference :) to some docs about it. So my memory about this 
being a chance to limit scope of lifetime of that temporary object was wrong 
then, so it's simply the same scope as the reference variable, not limited to 
the last line where the variable is actually used, did I get it right from 
reading that? (perhaps was some limit in some compiler, it's a decade ago that 
I was hinted to that, by accident remember the occasion, but less the content 
:) ).
If so, for the rest, seems we align in not using a const ref for the variable 
type, instead just making it the official variable holding the object/value, 
not just a reference. No gain with a const reference, and just confusing by 
being non-standard code.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> kossebau wrote in ftp.cpp:1376
> `QUrl::fileName()` returns a value QString, so just
> 
>   const QString filename = empurl.fileName();
> 
> While
> 
>   const QString  = empurl.fileName();
> 
> also is fine code IIRC, as I once learned to my surprise,  as the `const 
> QString &` here means to tell the compiler the actual value instance should 
> be hold only until the last use of the variable, not the end of the scope in 
> which the variable exists, this seems also not well known by others, so might 
> only make them confuse.
> Given this is an optimization not needed here, IMHO no need to use this 
> technique here.

IIUC, the compiler will use a temporary object to hold the return of 
tempurl.fileName().

The temporary is used to initialize filename, and then it's gone:
const QString filename = tempurl.filename();

filename here is a reference-to-const to the temporary object and the temporary 
will have the same lifetime as the reference (for objects on the stack), so 
until the end of the scope:
const QString  = tempurl.fileName();
c.f. 
https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

In my mind it kind of makes more sense to use a reference-to-const when the 
rvalue (tempurl.fileName()) is not a temporary object, because I am saving 
nothing by using a reference here since the compiler will create the temporary 
object and hold it until the end of the scope anyway.

In this particular case, it's probably exactly the same whether the temporary 
is used to initialize a const non-reference object and then (the temporary) is 
dropped/gone or the object being initialized is a reference to const to the 
temporary until the end of the scope...

job->statResult() and job->url() are different because both of them return a 
reference to const.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure
Cc: ahmadsamir, anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> ftp.cpp:1376
>  QString parentDir;
> -QString filename = tempurl.fileName();
> +const QString  = tempurl.fileName();
>  Q_ASSERT(!filename.isEmpty());

`QUrl::fileName()` returns a value QString, so just

  const QString filename = empurl.fileName();

While

  const QString  = empurl.fileName();

also is fine code IIRC, as I once learned to my surprise,  as the `const 
QString &` here means to tell the compiler the actual value instance should be 
hold only until the last use of the variable, not the end of the scope in which 
the variable exists, this seems also not well known by others, so might only 
make them confuse.
Given this is an optimization not needed here, IMHO no need to use this 
technique here.

> kossebau wrote in kcookiejar.cpp:1103
> 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.

Hm, why the change of all _domain to domain, not the other way around?
The _xyz naming of argument variables has been seen usually when it was needed 
to create a copy of the argument in the implementation, because e.g. the 
const-refness was in the way. As the normal naming of arguments and variables 
is without the _ prefix,. The prefix is then used with the argument name to 
denote this is the input to the actual variable then used in the implementation.

So:

  void foo(const Type arg)
   {
  // need to do non-const things with arg value, meh
  }

->

  void foo(const Type _arg)
   {
  Type arg(_arg);
  // do non-const things with arg value
  }

> kdirmodel.cpp:495
>  if (!urlsBeingFetched.isEmpty()) {
> -const QUrl dirUrl(url);
> +const QUrl (url);
>  for (const QUrl  : qAsConst(urlsBeingFetched)) {

`const QUrl  = url;` for consistency, please.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure
Cc: anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven added a comment.


  In D25039#557004 , @anthonyfieroni 
wrote:
  
  > Not using references is not a big problem with QString nor QUrl since they 
are reference counting objects, say if you don't change their content they are 
immutable, so
  >
  >   const QString s = other; // it's fine
  >   void func(QString s)
  >   {
  >const QString o = s; // use o instead of s is also fine, using plain 
s is fine too, if you don't touch mutability 
  >...
  >   }
  >
  
  
  Using cont &, you still save some reference counting overhead and allow the 
compiler to make potential better optimization.
  In your example you'd also save the boilerplate needed just to have a 
variable const.
  This also makes API clearer about what to expect.

INLINE COMMENTS

> kossebau wrote in ftp.cpp:1378
> 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 :)

I believe search can be removed in fact, fileName can be used interchangeably.
That's the const beauty it makes this kind of issue visisble.
But this patch is about fixing atomic warnings, not fixing code around those 
warnings.
This can be fixed in a subsequent diff, thanks for pointing it out.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure
Cc: anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25039: Fix Clazy performance issues, const

[Anthony Fieroni]

anthonyfieroni added a comment.


  Not using references is not a big problem with QString nor QUrl since they 
are reference counting objects, say if you don't change their content they are 
immutable, so
  
const QString s = other; // it's fine
void func(QString s)
{
 const QString o = s; // use o instead of s is also fine, using plain s 
is fine too, if you don't touch mutability 
 ...
}

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure
Cc: anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69083.
meven edited the summary of this revision.
meven added a comment.


  amend commit message

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69082=69083

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

To: meven, #frameworks, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven edited the summary of this revision.

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven edited the summary of this revision.

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69082.
meven edited the summary of this revision.
meven added a comment.


  amend commit message

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69081=69082

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

To: meven, #frameworks, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25039: Fix Clazy performance issues, const

[Méven Car]

meven retitled this revision from "Fix Clazy performance issues, const &, 
noexcept" to "Fix Clazy performance issues, const &".

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

D25039: Fix Clazy performance issues, const &, noexcept

[Méven Car]

meven updated this revision to Diff 69081.
meven marked 5 inline comments as done.
meven added a comment.


  Remove noexcept changes, avoid touching kpac, some code formatting

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=68955=69081

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

To: meven, #frameworks, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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 (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  = itu.key();

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

dito

> slavebase.h:363
>   */
> -QString configValue(QString key, const QString =QString()) 
> const;
> +QString configValue(const QString , const QString 
> =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  = 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);
>  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 )
>  {

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 


> sslui.cpp:87
>  QList errors;
> -for (const KSslError  : qAsConst(ud->sslErrors)) {
> +for (const QSslError  : 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

D25039: Fix Clazy performance issues, const &, noexcept

[Méven Car]

meven marked 2 inline comments as done.
meven added a comment.


  If somebody could accept this before KF 5.64 tagging, it would to modify 
`SlaveBase::configValue` signature that were added for the same value.
  I don't think any of this is subject to much concern.
  I may commit it without review.

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

D25039: Fix Clazy performance issues, const &, noexcept

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in slavebase.h:351
> Would be a proper change, but sadly also changes the signature of a API under 
> ABI promise, so here and in all other public API places not possibe.
> Can be only done when breaking the ABI with KF6.
> 
> Should there be something else done instead? IMHO no, neither adding a TODO 
> or even already an overload of the method with the preferred signature.
> For one, checking and improving all API of KF6 with clazy should be expected 
> to be done before any first release, so this should be catched at that time. 
> At the same time, this change is source-compatible, client-side code does not 
> have to be changed, so there is no real gain in client code readability, and 
> the runtime price tag of a flat QString copy here is equally small compared 
> to having another symbol and implementation code, so less code is better here.
> 
> See also 
> https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts

Ah, this is @since 5.64, so not yet published, so still can be changed.
 Sorry,  my comment was really drive-by quality #)

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

D25039: Fix Clazy performance issues, const &, noexcept

[Friedrich W. H. Kossebau]

kossebau added a comment.


  (Quick drive-by comment only as I had this in a search result...)

INLINE COMMENTS

> slavebase.h:351
>   */
> -bool configValue(QString key, bool defaultValue) const;
> +bool configValue(const QString , bool defaultValue) const;
>  

Would be a proper change, but sadly also changes the signature of a API under 
ABI promise, so here and in all other public API places not possibe.
Can be only done when breaking the ABI with KF6.

Should there be something else done instead? IMHO no, neither adding a TODO or 
even already an overload of the method with the preferred signature.
For one, checking and improving all API of KF6 with clazy should be expected to 
be done before any first release, so this should be catched at that time. At 
the same time, this change is source-compatible, client-side code does not have 
to be changed, so there is no real gain in client code readability, and the 
runtime price tag of a flat QString copy here is equally small compared to 
having another symbol and implementation code, so less code is better here.

See also 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts

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

D25039: Fix Clazy performance issues, const &, noexcept

[Méven Car]

meven created this revision.
meven added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  - Fix a lot of missing const & or &, avoiding copying mostly QString.
  - Add noexept to KFileItem and UDSEntre Move and copy constructors.
  - Make an implicit conversion explicit

TEST PLAN
  ctest

REPOSITORY
  R241 KIO

BRANCH
  clazy-perf

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/kpac/script.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp
  src/widgets/sslui.cpp

To: meven, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns