D24160: [KIO] Modernize the code to use range-for in more places

Tue Sep 24 15:28:48 BST 2019

kossebau added a comment.

  Not tested the changes myself, only looked at the code here.

  Looks good to me in general, modulo the comments I made.
  When it comes to `it`, my code reading expectations are that it is a real iterator, same like `i` is an integer index. The current patch proposes to also use it as loop argument name in some places, which I am not sure will help future code readers. I gave some proposals for names to use instead.

  Thanks for the work you already put into this, but also please go the full mile and move unrelated changes into separate commits :)

INLINE COMMENTS

> kcoredirlister.cpp:520
>      qCDebug(KIO_CORE_DIRLISTER) << "Iterating over dirs" << lstDirsCopy;
> -    for (QList<QUrl>::const_iterator it = lstDirsCopy.begin();
> -            it != lstDirsCopy.end(); ++it) {
> -        forgetDirs(lister, *it, false);
> +    for (const QUrl &it : lstDirsCopy) {
> +        forgetDirs(lister, it, false);

`it`->`dir`

> kcoredirlister.cpp:2303
>      const QStringList list = nameFilter.split(QLatin1Char(' '), QString::SkipEmptyParts);
> -    for (QStringList::const_iterator it = list.begin(); it != list.end(); ++it) {
> -        d->settings.lstFilters.append(QRegExp(*it, Qt::CaseInsensitive, QRegExp::Wildcard));
> +    for (const QString &it : list) {
> +        d->settings.lstFilters.append(QRegExp(it, Qt::CaseInsensitive, QRegExp::Wildcard));

`it`->`filterExpression`/`filter`/`expression`?

> kmountpoint.cpp:344
>              const QStringList mountDirs = gvfsDir.entryList(QDir::Dirs | QDir::NoDotAndDotDot);
> -            for (const auto &mountDir : mountDirs) {
> +            for (const QString &mountDir : mountDirs) {
>                  const QString type = mountDir.section(QLatin1Char(':'), 0, 0);

Why change away from auto?

> kmountpoint.cpp:429
>      KMountPoint::Ptr result;
> -    for (const_iterator it = begin(); it != end(); ++it) {
> -        const QString mountpoint = (*it)->d->mountPoint;
> +    for (const KMountPoint::Ptr &it : *this) {
> +        const QString mountpoint = it->d->mountPoint;

`it` -> `mountPointPtr`? Perfect would be renaming the existing `mountpoint` to `mountPointPath or something, so `mountPoint` could be used as loop var name. But I also think this would be too invasive WRT commit history and commit purpose.
Or `mp`, as used in some other code also looping over the same structure, see below in src/widgets/kpropertiesdialog.cpp

> kmountpoint.cpp:447
>      }
> -    for (const_iterator it = begin(); it != end(); ++it) {
> -        if (realDevice.compare((*it)->d->device, cs) == 0 ||
> -                realDevice.compare((*it)->d->mountedFrom, cs) == 0) {
> -            return *it;
> +    for (const KMountPoint::Ptr &it : *this) {
> +        if (realDevice.compare(it->d->device, cs) == 0 ||

`it`->`mountPoint`

> krecentdocument.cpp:170
>      QDir dir;
> -    for (QStringList::ConstIterator it = list.begin(); it != list.end(); ++it) {
> -        dir.remove(*it);
> +    for (const QString &it : list) {
> +        dir.remove(it);

`it`->`desktopFilePath` (hml, that `recentDocuments()` is semantically quite misleading without reading the docs).

> kdirsortfilterproxymodel.cpp:119
>  
> -    QFile::Permission permissionsCheck[] = { QFile::ReadUser,
> -                                             QFile::WriteUser,
> -                                             QFile::ExeUser,
> -                                             QFile::ReadGroup,
> -                                             QFile::WriteGroup,
> -                                             QFile::ExeGroup,
> -                                             QFile::ReadOther,
> -                                             QFile::WriteOther,
> -                                             QFile::ExeOther
> -                                           };
> -
> -    for (int i = 0; i < 9; i++) {
> -        points += info.permission(permissionsCheck[i]) ? 1 : 0;
> +    const QFile::Permission permissionsCheck[] = { QFile::ReadUser,
> +                                                   QFile::WriteUser,

Curious: while touching this, would a `static const` or `constexpr` not be the even better option?
Any rule of thumb known when telling the compiler which way to treat this?
Right now unsure what the compiler even does with a plain const array in a method without any optimization. It copies the array data onto the stack on the method invocation, or?

> kio_help.cpp:69
>      for (int id = 0; id < ldCount; id++) {
> -        QStringList::ConstIterator lang;
> -        for (lang = langs.constBegin(); lang != langs.constEnd(); ++lang) {
> -            search.append(QStringLiteral("%1/%2/%3").arg(localDoc[id], *lang, fname));
> +        for(const QString &lang : qAsConst(langs)) {
> +            search.append(QStringLiteral("%1/%2/%3").arg(localDoc[id], lang, fname));

missing space after for

> kio_trash.cpp:503
>      listEntry(entry);
> -    for (TrashedFileInfoList::ConstIterator it = lst.begin(); it != lst.end(); ++it) {
> -        const QUrl url = TrashImpl::makeURL((*it).trashId, (*it).fileId, QString());
> +    for (const TrashedFileInfo &it : lst) {
> +        const QUrl url = TrashImpl::makeURL(it.trashId, it.fileId, QString());

`it`->`info`

> trashimpl.cpp:1093
>      const QList<Solid::Device> lst = Solid::Device::listFromQuery(QStringLiteral("StorageAccess.accessible == true"));
> -    for (QList<Solid::Device>::ConstIterator it = lst.begin(); it != lst.end(); ++it) {
> -        QString topdir = (*it).as<Solid::StorageAccess>()->filePath();
> +    for (const Solid::Device &it : lst) {
> +        QString topdir = it.as<Solid::StorageAccess>()->filePath();

`it` -> `device`

> kuriikwsfiltereng.cpp:304
>            QString c = rlitem.left(1);
> -          if (c== QLatin1Char('0'))
> +          if (c == QLatin1Char('0'))
>            {

unrelated change, please separate commit

> kuriikwsfiltereng.cpp:311
>            }
> -          else if ((c>=QLatin1String("0")) && (c<=QLatin1String("9"))) // krazy:excludeall=doublequote_chars
> +          else if (c >= QLatin1Char('0') && c <= QLatin1Char('9'))
>            {

Unrelated change/fix, please separate commit. Though, is this even correct? There is no `QString::operator>=(QChar)` & similar, is there?

> kuriikwsfiltereng.cpp:315
>              int n = rlitem.toInt();
> -            ql[n-1] = QLatin1String("");
> +            ql[n-1].clear();
>            }

Unrelated, please separate commit. Was the code checked that isNull() is not relevant?

> kshellcompletion.cpp:145
> +    for (auto &match : *matches) {
> +        if (!match.value().isNull()) {
> +            if (match.value().endsWith(QLatin1Char('/'))) {

While touching all the lines, would improve the code to cache `match.value()` into some
`QString& matchString = match.value();` IMHO.

> kurifilter.cpp:681
>  
> -    for (const KPluginMetaData &pluginMetaData : plugins) {
> +    for (const KPluginMetaData &pluginMetaData : qAsConst(plugins)) {
>          const QString fileName = pluginMetaData.fileName().section(QLatin1Char('/'), -1);