As the title says. In my humble opinion this makes the code *much* easier to read.
Details
- Reviewers
aacid - Group Reviewers
Okular - Commits
- R223:f34ebf659f6c: Replace some iterator loops by range-based for
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.
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.
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".
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 |
Fine with me, too. If somebody is in favour of auto, please say so now. Otherwise I will remove the autos from the patch.
ui/pageview.cpp | ||
---|---|---|
941 | This code is suddenly less performant than the old one. |
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 |
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.