D25755: Replace iterators with range-based for

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: dhaumann, dfaure, ahmadsamir, broulik, aacid, apol, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D25755: Replace iterators with range-based for

[Dominik Haumann]

dhaumann added a comment.


  >>> 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 ` and `const QString `, 
i.e. references and not copies.

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: dhaumann, dfaure, ahmadsamir, broulik, aacid, apol, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D25755: Replace iterators with range-based for

[Ahmad Samir]

ahmadsamir added a subscriber: dfaure.

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: dfaure, ahmadsamir, broulik, aacid, apol, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns

D25755: Replace iterators with range-based for

[Ahmad Samir]

ahmadsamir added a comment.


  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.

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: ahmadsamir, broulik, aacid, apol, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25755: Replace iterators with range-based for

[Aleix Pol Gonzalez]

apol added a comment.


  In D25755#572575 , @broulik wrote:
  
  > In D25755#572418 , @apol wrote:
  >
  > > 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.

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: broulik, aacid, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25755: Replace iterators with range-based for

[Kai Uwe Broulik]

broulik added a comment.


  In D25755#572418 , @apol wrote:
  
  > 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 :)

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: broulik, aacid, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25755: Replace iterators with range-based for

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> kded.cpp:622
>  const QStringList dirs = 
> QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, 
> QStringLiteral("kconf_update"), QStandardPaths::LocateDirectory);
> -for (QStringList::ConstIterator it = dirs.begin();
> -it != dirs.end();
> -++it) {
> -QString path = *it;
> +for (const QString dir : dirs) {
> +QString path = dir;

& for dir

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: aacid, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25755: Replace iterators with range-based for

[Aleix Pol Gonzalez]

apol added a comment.


  Do we dislike iterators now?

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25755: Replace iterators with range-based for

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

TEST PLAN
  builds, kded starts

REPOSITORY
  R297 KDED

BRANCH
  iter

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

AFFECTED FILES
  src/kded.cpp

To: nicolasfella, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns