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

[David Faure]

dfaure marked 18 inline comments as done.
dfaure added inline comments.

INLINE COMMENTS

> kossebau wrote in kmountpoint.cpp:344
> Why change away from auto?

Consistency with all other range for loops in kio, and https://wiki.qt.io/Coding_Conventions#auto_Keyword

> kossebau wrote in kmountpoint.cpp:429
> `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

picked `mp`

> kossebau wrote in kdirsortfilterproxymodel.cpp:119
> 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?

Good question.

Without optimizations, I assume `static` means generating atomics for thread-safety, while on stack generates a `memcpy` indeed. 
I checked the assembly on godbolt.org, for that last part. Couldn't see atomics with static because the testcase is all in main(), so no threads involved, but in a library method this is a requirement.

With -O2, the generated code is exactly the same.
https://godbolt.org/z/Ez8fba

So I guess it doesn't matter?

> kossebau wrote in kuriikwsfiltereng.cpp:304
> unrelated change, please separate commit

Indeed, this whole subdir was in need of code reformatting.
Done, https://commits.kde.org/kio/c8c2eff2a2362278acddcc995221176a7788e24e

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

Oh indeed, I thought `c` was a QChar, given its name...

I guess this creates a QString from the QChar but yeah I'll revert.

> kuriikwsfiltereng.cpp:324
>              if (it != ql.end())
>                it->clear();
>            }

the code here uses clear() already for `ql` elements.

> kuriikwsfiltereng.cpp:346
>        // Generate list of unmatched strings:
>        QString v = ql.join(QLatin1Char(' ')).simplified();
>  

and this is the final use of `ql`, null vs empty makes no difference =>
https://commits.kde.org/kio/ea035cd204dea8cd563155c4256400fe67e8b69c

> dropjob.cpp:67
>      explicit DropMenu(QWidget *parent = nullptr);
> -    ~DropMenu();
> +    ~DropMenu() override;
>  

Moved all overrides to https://commits.kde.org/kio/54429f8a36aad65088b02eb9d13c3bf78f61f22f

> kossebau wrote in kurifilter.cpp:681
> I would make this a separate commit, as this does not exactly match the commit message and might confuse future history readers.

https://commits.kde.org/kio/1660e30f3821204b08b43910738fa52ce1cf0915

REPOSITORY
  R241 KIO

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

To: dfaure, bruns, kossebau
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/20190928/18555971/attachment-0001.html>