This was done using clang-tidy's modernize-loop-convert, then
clazy (for qAsConst), then manual cleanups (less auto, better variable
names, etc). Took way longer than I was hoping for...
Details
make && ctest
Diff Detail
- Repository
- R241 KIO
- Branch
- 2019_09_modernize_range_for
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 17125 Build 17143: arc lint + arc unit
Will see to give this some review later this week (if still needed). Would agree the "modern" code is more easy to grasp by humans and thus better to maintain, so +1 for principle.
All override changes please as separate commit though, unrelated to commit message (IMHO can go in directly). Do we already have agreed on using override with destructors in KF code I do prefer them as well).
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 :)
src/core/kcoredirlister.cpp | ||
---|---|---|
520–521 | it->dir | |
2303–2304 | it->filterExpression/filter/expression? | |
src/core/kmountpoint.cpp | ||
344 | Why change away from auto? | |
429–430 | 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. | |
447–450 | it->mountPoint | |
src/core/krecentdocument.cpp | ||
170–171 | it->desktopFilePath (hml, that recentDocuments() is semantically quite misleading without reading the docs). | |
src/filewidgets/kdirsortfilterproxymodel.cpp | ||
119–131 | Curious: while touching this, would a static const or constexpr not be the even better option? | |
src/ioslaves/help/kio_help.cpp | ||
69–70 | missing space after for | |
src/ioslaves/trash/kio_trash.cpp | ||
503–504 | it->info | |
src/ioslaves/trash/trashimpl.cpp | ||
1093–1094 | it -> device | |
src/urifilters/ikws/kuriikwsfiltereng.cpp | ||
304 ↗ | (On Diff #66665) | unrelated change, please separate commit |
311 ↗ | (On Diff #66665) | Unrelated change/fix, please separate commit. Though, is this even correct? There is no QString::operator>=(QChar) & similar, is there? |
315 ↗ | (On Diff #66665) | Unrelated, please separate commit. Was the code checked that isNull() is not relevant? |
src/widgets/kshellcompletion.cpp | ||
145 | While touching all the lines, would improve the code to cache match.value() into some | |
src/widgets/kurifilter.cpp | ||
681 ↗ | (On Diff #66665) | I would make this a separate commit, as this does not exactly match the commit message and might confuse future history readers. |
src/widgets/previewjob.cpp | ||
760 | while touching all the lines & vars, desktopEntryName() could be cached. | |
774 | I would make this a separate commit, as this does not exactly match the commit message and might confuse future history readers. | |
tests/getalltest.cpp | ||
17 ↗ | (On Diff #66665) | I would make this a separate commit, as this does not exactly match the commit message and might confuse future history readers. |
33 ↗ | (On Diff #66665) | I would make this a separate commit, as this does not exactly match the commit message and might confuse future history readers. |
tests/previewtest.cpp | ||
54 ↗ | (On Diff #66665) | I would make this a separate commit, as this does not exactly match the commit message and might confuse future history readers. |
src/core/kmountpoint.cpp | ||
---|---|---|
344 | Consistency with all other range for loops in kio, and https://wiki.qt.io/Coding_Conventions#auto_Keyword | |
429–430 | picked mp | |
src/filewidgets/kdirsortfilterproxymodel.cpp | ||
119–131 | Good question. Without optimizations, I assume static means generating atomics for thread-safety, while on stack generates a memcpy indeed. With -O2, the generated code is exactly the same. So I guess it doesn't matter? | |
src/urifilters/ikws/kuriikwsfiltereng.cpp | ||
304 ↗ | (On Diff #66665) | Indeed, this whole subdir was in need of code reformatting. |
311 ↗ | (On Diff #66665) | 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. |
324 ↗ | (On Diff #66665) | the code here uses clear() already for ql elements. |
346 ↗ | (On Diff #66665) | and this is the final use of ql, null vs empty makes no difference => |
src/widgets/dropjob.cpp | ||
67 ↗ | (On Diff #66665) | Moved all overrides to https://commits.kde.org/kio/54429f8a36aad65088b02eb9d13c3bf78f61f22f |
src/widgets/kurifilter.cpp | ||
681 ↗ | (On Diff #66665) |
Code changes look all fine to me. Only looked at code here with eyes (tired and fermentation products influenced ;) ), not built, run or tested though.