- Fix a lot of missing const & or &, avoiding copying mostly QString.
Details
- Reviewers
dfaure kossebau - Group Reviewers
Frameworks - Commits
- R241:f2a3a78972b2: Fix Clazy performance issues, const &
ctest
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
(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. 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. See also https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts |
src/core/slavebase.h | ||
---|---|---|
351 | Ah, this is @since 5.64, so not yet published, so still can be changed. |
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? 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). | |
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. | |
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. |
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. |
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. | |
src/ioslaves/http/kcookiejar/kcookiejar.cpp | ||
1102–1103 | Hm, why the change of all _domain to domain, not the other way around? 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. |
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: 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: 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. |
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 :) ). |
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 ↗ | (On Diff #69121) | As there is no override changed in this patch, I found this suspicious and had a look: Nothing is using this method anymore now, so it can instead be simply removed, in a separate commit. |
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.