[chmodjob] Port away from QLinkedList
ClosedPublic

Authored by nicolasfella on Nov 21 2019, 4:35 PM.

Details

Summary

QLinkedList is likely to go away in Qt 6. Use a std::stack instead

Test Plan

Changed file permissions in Dolphin, verified with ls -l

Diff Detail

Repository
R241 KIO
Branch
ll
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19045
Build 19063: arc lint + arc unit
nicolasfella created this revision.Nov 21 2019, 4:35 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 21 2019, 4:35 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
nicolasfella requested review of this revision.Nov 21 2019, 4:35 PM

While the change should be fine as is, that sounds like a job for std::deque, as we don't need insertions/removals at random positions. Or even for a std::queue, because we only operate on the front.

bruns added a subscriber: bruns.Nov 21 2019, 8:05 PM

Or just change the direction?

src/core/chmodjob.cpp
115

For efficiency, probably do a
m_infos.reserve(m_infos.capacity() + m_lstItems.size());
here.

155

For efficiency, probably do a
m_infos.reserve(m_infos.capacity() + list.size());
here.

dfaure added a subscriber: dfaure.Nov 21 2019, 10:01 PM
dfaure added inline comments.
src/core/chmodjob.cpp
131

This could even be push_front(std::move(info)) since info isn't used anymore afterwards;

190

same here

224

same here

nicolasfella marked 3 inline comments as done.Nov 22 2019, 8:45 AM
nicolasfella added inline comments.
src/core/chmodjob.cpp
115

Does that make sense with a linked list?

Indeed there's no reserve for std::list.

And std::queue is FIFO, not LIFO.

std::stack would be a more appropriate container here.

  • Use stack
dfaure requested changes to this revision.Jan 1 2020, 10:17 PM

Code looks good, just the comment needs a fix.

[Too bad std::stack doesn't seem to have a reserve() method.]

src/core/chmodjob.cpp
188

"Prepend" is confusing now since push is more of an append. This comment needs an update, like "Push this info on top of the stack so it's handled first"

This revision now requires changes to proceed.Jan 1 2020, 10:17 PM
  • Update comment
nicolasfella marked an inline comment as done.Apr 14 2020, 4:50 PM
dfaure accepted this revision.Apr 14 2020, 8:31 PM

(remember to s/list/stack/ in the commit log)

This revision is now accepted and ready to land.Apr 14 2020, 8:31 PM
nicolasfella edited the summary of this revision. (Show Details)Apr 14 2020, 8:51 PM
This revision was automatically updated to reflect the committed changes.