Differential D24171

[src/filewidgets/*] replace deprecated foreach with range for
ClosedPublic
Actions

Authored by ahmadsamir on Sep 23 2019, 7:13 PM.

Details

Reviewers
kde-frameworks-devel
dfaure
Commits
R241:68b7a22f9e0e: [src/filewidgets/*] replace deprecated foreach with range for
Test Plan

The code compiles and tests pass

Diff Detail

Repository
R241 KIO
Branch
ahmad/foreach-filewidgets (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17136
Build 17154: arc lint + arc unit
ahmadsamir created this revision.Sep 23 2019, 7:13 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 23 2019, 7:13 PM
ahmadsamir requested review of this revision.Sep 23 2019, 7:13 PM
dfaure added a comment.Sep 23 2019, 9:54 PM

Cool, I'll review this. BTW don't touch begin/end loops, that's done in D24160 already.

ahmadsamir added a comment.Sep 23 2019, 10:05 PM
In D24171#536712, @dfaure wrote:

Cool, I'll review this. BTW don't touch begin/end loops, that's done in D24160 already.

Yep, I saw it on the ML :-)

ahmadsamir updated this revision to Diff 66899.Sep 26 2019, 3:26 PM

More implicit sharing detach prevention

dfaure requested changes to this revision.Sep 28 2019, 11:18 AM
dfaure added inline comments.
src/filewidgets/kfilepreviewgenerator.cpp
732–733

SLOW. This should actually use iterators.

src/filewidgets/knewfilemenu.cpp
1222
qDeleteAll(actions);
src/filewidgets/kurlnavigator.cpp
803

Why not a range for here? It should be just fine.

This revision now requires changes to proceed.Sep 28 2019, 11:18 AM
ahmadsamir marked 2 inline comments as done.Sep 28 2019, 6:49 PM
ahmadsamir added inline comments.
src/filewidgets/kurlnavigator.cpp
803

I thought button->deleteLater() may change m_navButtons, doesn't it?

dfaure added inline comments.Sep 28 2019, 6:58 PM
src/filewidgets/kurlnavigator.cpp
803

How would it? It doesn't know that container. All it does is posting an event to that object. The container isn't modified.

[and even a real C++ delete wouldn't modify the container itself, only the objects it points to]

ahmadsamir marked an inline comment as done.Sep 28 2019, 8:02 PM
ahmadsamir updated this revision to Diff 67007.Sep 28 2019, 8:03 PM

Change according to comments

dfaure added inline comments.Sep 28 2019, 8:08 PM
src/filewidgets/kfilepreviewgenerator.cpp
733 ↗(On Diff #67007)

keys() is what is slow (it creates and fills a temporary container)

You want m_cutItemsCache.cbegin()

ahmadsamir updated this revision to Diff 67008.Sep 28 2019, 8:28 PM

Don't iterate over the temp. container returned by qhash.keys(), rather over the qhash itself

dfaure accepted this revision.Sep 29 2019, 8:17 PM

Thanks!

This revision is now accepted and ready to land.Sep 29 2019, 8:17 PM
ahmadsamir added a comment.Sep 29 2019, 8:26 PM

(Reminder: I don't have a dev account so you'll have to commit the patches :)).

dfaure added a comment.Sep 29 2019, 8:30 PM
In D24171#539612, @ahmadsamir wrote:

(Reminder: I don't have a dev account so you'll have to commit the patches :)).

If you keep this up, you should ask for one :-)

dfaure closed this revision.Sep 29 2019, 8:33 PM

Showing Only Differences

This revision modifies 4 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

Diff 67008

View Options

src/filewidgets/kfilepreviewgenerator.cpp

Loading...
View Options

src/filewidgets/knewfilemenu.cpp

Loading...
View Options

src/filewidgets/kurlnavigator.cpp

Loading...