Replace some iterator loops by range-based for
ClosedPublic

Authored by sander on Mar 5 2019, 11:03 AM.

Details

Summary

As the title says. In my humble opinion this makes the code *much* easier to read.

Test Plan

No functional changes.

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sander created this revision.Mar 5 2019, 11:03 AM
Restricted Application added a project: Okular. · View Herald TranscriptMar 5 2019, 11:03 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
sander requested review of this revision.Mar 5 2019, 11:03 AM
davidhurka added a subscriber: davidhurka.EditedMar 5 2019, 12:41 PM

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 ); calls PageViewItem::PageViewItem( auto ). But does auto stand for Okular::Page * or int? And later, should I write new PageViewItem( page ) or new PageViewItem( pageSet.at( page ) )?

But yes, it’s much easier to read.

sander added a comment.Mar 5 2019, 1:23 PM

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".

aacid added a subscriber: aacid.Mar 5 2019, 5:48 PM

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

ui/presentationwidget.cpp
313

qDeleteAll

sander added a comment.Mar 5 2019, 7:38 PM

Fine with me, too. If somebody is in favour of auto, please say so now. Otherwise I will remove the autos from the patch.

sander updated this revision to Diff 53294.Mar 6 2019, 4:27 PM
  • Do not use auto in range-based for
  • Use qDeleteAll where appropriate
aacid added inline comments.Mar 6 2019, 6:40 PM
ui/pageview.cpp
941

This code is suddenly less performant than the old one.

Read https://doc.qt.io/qt-5/qtglobal.html#qAsConst

sander updated this revision to Diff 53383.Mar 7 2019, 6:23 PM

Ugly; didn't know about 'hidden detaches'.

New patch with qAsConst.

Albert, is this okay now?

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

Ok, now that i did that, a few of the don't need qAsConst, adding it doesn't make it worse, but it's a bit weird so i'd prefer if you removed it.

ui/pageview.cpp
1090

you don't need qAsConst, pageSet is already const as defined in the function signature.

1098

You don't need qAsConst here, pageFields is already const as defined in the line above.

ui/presentationwidget.cpp
332

pageset is const, please remove qAsConst.

337

annotations is const, please remove qasconst

sander updated this revision to Diff 53942.Mar 15 2019, 8:53 AM

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't know that) I'll try to use arc next time.

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't know that) I'll try to use arc next time.

Well, it's much nicer because you can see the context of the files instead of those ugly "Context not available." markers.

But if you have to waste time so that i save it, it's not a net gain in the end. I was justifying my lateness in reviewing this, since it doesn't make it easy for me it goes a bit to the bottom of the stack.

aacid accepted this revision.Mar 26 2019, 10:47 PM
This revision is now accepted and ready to land.Mar 26 2019, 10:47 PM
This revision was automatically updated to reflect the committed changes.