builds, kded starts
Details
Details
Diff Detail
Diff Detail
- Repository
- R297 KDED
- Branch
- iter
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 19503 Build 19521: arc lint + arc unit
| src/kded.cpp | ||
|---|---|---|
| 622 | & for dir | |
Comment Actions
We don't, and they still make sense for when you need the key, but range for is just much nier to look at :)
Comment Actions
I'm fine with that statement. But are we going to be reviewing changing all the KDE code from iterators to range for? Feels like an overkill to me.
Comment Actions
If we're going to iterate over a whole container, from cbegin() to cend(), and the container isn't going to change in the loop, then IMHO (H for humble :)) range-for would convey the intention better and make the code slightly easier to read/look cleaner.
Kind of like one of the main reasons foreach was used (in, as we're finding out now that we're removing it from KDE code, many many places) instead of iterator/index based for loops, to begin with.
EDIT: clang-tidy, clazy, then eyeballing could be used according to @dfaure, see D24160
Comment Actions
Do we dislike iterators now?
We don't, and they still make sense for when you need the key, but range for is just much nier to look at :)
I'm fine with that statement. But are we going to be reviewing changing all the KDE code from iterators to range for? Feels like an overkill to me.
It's much more than just the fact "it's nicer to look at". It' i) more compact, but more importantly, it's ii) simpler to reason about. When having iterator-based loops you cannot immediately say "we iterate over all items". It could be that the iterator is not increased, or it is increased in the body scope multiple times. Contrary, with range-based for loops you know what you *always* iterate over all items (except if there is a shortcut break/return statement).
The only nitpick is: In the Qt world this is sometimes tricky when using range-based for loops over non-const Qt containers, which leads to a detach. So review is typically necessary.
PS: Yes, it should be const QString &dir and const QString &directory, i.e. references and not copies.
Comment Actions
https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
is something you can use if you feel bored, i made it mandatory in poppler