D23902: [KCoreDirLister] replace foreach with range-for

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoredirlister.cpp:285
>  {
> -    Q_FOREACH (CachedItemsJob *job, m_cachedItemsJobs) {
> +    for (CachedItemsJob *job : qAsConst(m_cachedItemsJobs)) {
>          if (job->url() == url) {

The method is const, so this qAsConst isn't needed.

> kcoredirlister.cpp:1044
>  
> -QList<QUrl> KCoreDirListerCache::directoriesForCanonicalPath(const QUrl &dir) const
> +const QList<QUrl> KCoreDirListerCache::directoriesForCanonicalPath(const QUrl &dir) const
>  {

It's generally considered bad practice to return a const value, because the caller makes a copy anyway, so this doesn't guarantee anything. And it removes the benefits of rvalues that can be moved, since C++11.

I can see how range-for actually benefits from this, though.

It just seems that the generally agreed solution is to return non-const (for the other benefits) and use a local const variable as intermediary when using this in a range-for.

> kcoredirlister.cpp:1474
>          // TODO: make this a separate method?
> -        foreach (KCoreDirLister *kdl, listers + holders) {
> +        for (KCoreDirLister *kdl : listers + holders) {
>              if (kdl->d->rootFileItem.isNull() && kdl->d->url == newUrl) {

I guess this needs a local const var to hold listers+holders, so that the container that we're iterating on, is const?

> kcoredirlister.cpp:1492
>          // emit old items: listers, holders
> -        foreach (KCoreDirLister *kdl, listers + holders) {
> +        for (KCoreDirLister *kdl : listers + holders) {
>              if (kdl->d->rootFileItem.isNull() && kdl->d->url == newUrl) {

same here

> kcoredirlister.cpp:1960
>              // we need a copy because stop modifies the list
>              QList<KCoreDirLister *> listers = (*dit).listersCurrentlyListing;
> +            for (KCoreDirLister *kdl : listers) {

const

> kcoredirlister.cpp:1968
>              // we need a copy because forgetDirs modifies the list
>              QList<KCoreDirLister *> holders = (*dit).listersCurrentlyHolding;
> +            for (KCoreDirLister *kdl : holders) {

const

> kcoredirlister.cpp:2073
>          }
> -        qCDebug(KIO_CORE_DIRLISTER) << "  " << dit.key() << (*dit).listersCurrentlyHolding.count() << "holders:" << list;
> +        qCDebug(KIO_CORE_DIRLISTER) << "  " << dit.key() << holders << "holders:" << list;
>      }

`holders.count()`, to match what the orig code was doing

> kcoredirlister.cpp:2222
> +    const QList<QUrl> dirs = lstDirs;
> +    for (const QUrl &dir : dirs) {
> +        const QList<KFileItem> *itemList = kDirListerCache()->itemsForDir(dir);

Why not just `qAsConst(listDirs)`? Did you identify a risk that the body of the for loop modifies lstDirs?

REPOSITORY
  R241 KIO

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

To: ahmadsamir, kde-frameworks-devel, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190914/c830dafb/attachment-0001.html>