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
