Fix Clazy performance issues, const &
ClosedPublic

Authored by meven on Oct 29 2019, 9:20 AM.

Details

Summary
  • Fix a lot of missing const & or &, avoiding copying mostly QString.
Test Plan

ctest

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D25039
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18398
Build 18416: arc lint + arc unit
meven created this revision.Oct 29 2019, 9:20 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 29 2019, 9:20 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Oct 29 2019, 9:20 AM

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

src/core/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

kossebau added inline comments.Oct 30 2019, 6:46 PM
src/core/slavebase.h
351

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

meven marked 2 inline comments as done.Oct 30 2019, 7:12 PM

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.

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.

src/core/kcoredirlister.cpp
1551

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();
1944

dito

src/core/slavebase.h
363

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

src/ioslaves/ftp/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 :)

src/ioslaves/http/kcookiejar/kcookiejar.cpp
1102–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.

src/kpac/script.cpp
201 ↗(On Diff #68955)

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

src/widgets/sslui.cpp
87 ↗(On Diff #68955)

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

meven updated this revision to Diff 69081.Oct 31 2019, 6:18 AM
meven marked 5 inline comments as done.

Remove noexcept changes, avoid touching kpac, some code formatting

meven retitled this revision from Fix Clazy performance issues, const &, noexcept to Fix Clazy performance issues, const &.Oct 31 2019, 6:19 AM
meven edited the summary of this revision. (Show Details)
meven updated this revision to Diff 69082.Oct 31 2019, 6:19 AM
meven edited the summary of this revision. (Show Details)

amend commit message

meven edited the summary of this revision. (Show Details)Oct 31 2019, 6:28 AM
meven updated this revision to Diff 69083.Oct 31 2019, 6:29 AM
meven edited the summary of this revision. (Show Details)

amend commit message

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 
     ...
}
meven added a comment.Oct 31 2019, 7:40 AM

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.

src/ioslaves/ftp/ftp.cpp
1378

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.

kossebau added inline comments.Oct 31 2019, 2:50 PM
src/ioslaves/ftp/ftp.cpp
1376

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

const QString filename = empurl.fileName();

While

const QString &filename = 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.

src/ioslaves/http/kcookiejar/kcookiejar.cpp
1102–1103

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
}
src/widgets/kdirmodel.cpp
495

const QUrl &dirUrl = url; for consistency, please.

ahmadsamir added inline comments.
src/ioslaves/ftp/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 &filename = 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.

kossebau added inline comments.Oct 31 2019, 4:35 PM
src/ioslaves/ftp/ftp.cpp
1376

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.

meven updated this revision to Diff 69121.Oct 31 2019, 4:42 PM
meven marked 7 inline comments as done.

Review feedback

kossebau accepted this revision.Nov 1 2019, 3:02 PM

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)

src/ioslaves/http/kcookiejar/kcookieserver.h
106

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.

This revision is now accepted and ready to land.Nov 1 2019, 3:02 PM
meven added a comment.Nov 1 2019, 3:31 PM

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.

Feel free to do the code removal,

Done now, as I had the files still open.

meven updated this revision to Diff 69178.Nov 2 2019, 7:16 AM

Rebasing on master

This revision was automatically updated to reflect the committed changes.