D19539: Replace some iterator loops by range-based for

This revision was automatically updated to reflect the committed changes. Closed by commit R223:f34ebf659f6c: Replace some iterator loops by range-based for (authored by sander, committed by aacid). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D19539?vs=53942&id=54900#toc REPOSITORY R

D19539: Replace some iterator loops by range-based for

aacid accepted this revision. This revision is now accepted and ready to land. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D19539 To: sander, #okular, aacid Cc: aacid, davidhurka, okular-devel, joaonetto, tfella, ngraham, darcyshen

D19539: Replace some iterator loops by range-based for

aacid added a comment. In D19539#431446 , @sander wrote: > Removed those four `aAsConst` again. > > I have been shying away from `arc` because I didn't want to learn yet another tool just for those few Okular patches that I submit. But if th

D19539: Replace some iterator loops by range-based for

sander updated this revision to Diff 53942. sander added a comment. Removed those four `aAsConst` again. I have been shying away from `arc` because I didn't want to learn yet another tool just for those few Okular patches that I submit. But if that makes *your* life more difficult (didn

D19539: Replace some iterator loops by range-based for

aacid added a comment. Sorry it did take a while to asnwer, since you're not using arc i can't use phabricator to navigate the code and have to find time to sit don't and check whether some of those variables were const or not because their declaration was outside the lines of the diff

D19539: Replace some iterator loops by range-based for

sander added a comment. Albert, is this okay now? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D19539 To: sander, #okular Cc: aacid, davidhurka, okular-devel, tfella, ngraham, darcyshen

D19539: Replace some iterator loops by range-based for

sander updated this revision to Diff 53383. sander added a comment. Ugly; didn't know about 'hidden detaches'. New patch with `qAsConst`. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19539?vs=53294&id=53383 REVISION DETAIL https://phabricator.kde.o

D19539: Replace some iterator loops by range-based for

aacid added inline comments. INLINE COMMENTS > pageview.cpp:941 > { > -QVector< PageViewItem * >::const_iterator it = d->items.constBegin(), > itEnd = d->items.constEnd(); > -for ( ; it < itEnd; ++it ) > +for ( PageViewItem * item : d->items ) > { This code is suddenly less pe

D19539: Replace some iterator loops by range-based for

sander updated this revision to Diff 53294. sander added a comment. - Do not use `auto` in range-based for - Use `qDeleteAll` where appropriate REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19539?vs=53191&id=53294 REVISION DETAIL https://phabricator.kd

D19539: Replace some iterator loops by range-based for

sander added a comment. Fine with me, too. If somebody is in favour of `auto`, please say so now. Otherwise I will remove the `auto`s from the patch. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D19539 To: sander, #okular Cc: aacid, davidhurka, okular-devel, tfel

D19539: Replace some iterator loops by range-based for

aacid added a comment. auto makes the code much more unreadable for me, i don't want to have to go to the header to figure out which type the variable is INLINE COMMENTS > presentationwidget.cpp:313 > // delete frames > -QVector< PresentationFrame * >::iterator fIt = m_frames.begin(

D19539: Replace some iterator loops by range-based for

sander added a comment. You may want to read up on range-based for; it is very helpful. In such a for-loop, `auto` always means "the type of a container entry". REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D19539 To: sander, #okular Cc: davidhurka, okular-devel, t

D19539: Replace some iterator loops by range-based for

davidhurka added a comment. Isn’t `auto` a bit confusing as item type? A beginner like me wouldn’t understand that. Example (pageview.cpp:1194): `page` is an item of `pageSet`, which is `QVector< Okular::Page * >`. If I don’t know that: `PageViewItem * item = new PageViewItem( page )

D19539: Replace some iterator loops by range-based for

sander created this revision. sander added a reviewer: Okular. Herald added a project: Okular. Herald added a subscriber: okular-devel. sander requested review of this revision. REVISION SUMMARY As the title says. In my humble opinion this makes the code *much* easier to read. TEST PLAN No