Runners: Convert foreach to for
ClosedPublic

Authored by meven on Jan 5 2020, 5:11 PM.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Jan 5 2020, 5:11 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 5 2020, 5:11 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Jan 5 2020, 5:11 PM
davidedmundson accepted this revision.Jan 5 2020, 9:38 PM
davidedmundson added a subscriber: davidedmundson.

Cool, thanks

runners/bookmarks/fetchsqlite.cpp
99–100

There's an optimisation available here

runners/windows/windowsrunner.cpp
78–79

Does this detach?

This revision is now accepted and ready to land.Jan 5 2020, 9:38 PM
meven updated this revision to Diff 72864.Jan 6 2020, 10:33 AM
meven marked an inline comment as done.

Ensure windowList is const, use stdMap to loop over QMap key+value

davidedmundson accepted this revision.Jan 6 2020, 10:34 AM
meven updated this revision to Diff 72866.Jan 6 2020, 10:35 AM
meven marked an inline comment as done.

Add a ref

ivan requested changes to this revision.Jan 6 2020, 10:39 AM
ivan added a subscriber: ivan.
ivan added inline comments.
runners/bookmarks/fetchsqlite.cpp
99–100

This can be made much better by providing a custom range object to iterate from keyValueBegin() to keyValueEnd() - converting to std::map is quite inefficient.

This revision now requires changes to proceed.Jan 6 2020, 10:39 AM
ivan added a comment.Jan 6 2020, 10:47 AM

Something like this:

template <typename T>
class asRangeImpl
{
public:
    asRange(const T &data)
        : m_data{data}
    {
    }

    auto begin() const { return m_data.keyValueBegin(); }
    auto end() const { return m_data.keyValueEnd(); }

private:
    const T &m_data;
};

template <typename T>
class asRange(const T& map)
{
    return asRangeImpl<T>(map);
}

We don't need any custom code, it's a QMap, it has existing iterators already which can be used.

meven updated this revision to Diff 72867.Jan 6 2020, 11:02 AM
meven marked an inline comment as done.

Use a loop over bindObjects.constKeyValueBegin/bindObjects.constKeyValueEnd

ivan requested changes to this revision.Jan 6 2020, 11:05 AM

@davidedmundson

Yes, the wrapper just serves for keyval iterators to be usable with a range-based for loop. Quite useful if there are a lot of these. In this case, where it is only one loop I agree it might be an overkill.

@meven

Just a single nit-pick left :)

runners/bookmarks/fetchsqlite.cpp
101

entry->first and entry->second

This revision now requires changes to proceed.Jan 6 2020, 11:05 AM
meven added a comment.Jan 6 2020, 12:30 PM
In D26438#588598, @ivan wrote:

@davidedmundson

Yes, the wrapper just serves for keyval iterators to be usable with a range-based for loop. Quite useful if there are a lot of these. In this case, where it is only one loop I agree it might be an overkill.

I wish your wrapper was part of Qt directly to avoid boilerplate and give a simpler more idimatic way to get a key/value iteration.

@meven

Just a single nit-pick left :)

You suggestion does not work (or at least QtCreator does not allow me to write like you suggested).
entry here is a QKeyValueIterator that needs to be dereferenced using std::pair<Key, T> QKeyValueIterator::operator*() to get its underlining std::pair.

ivan accepted this revision.Jan 6 2020, 12:38 PM

@meven

I hate Qt... defining operator* without operator-> ... ugh

I've approved the patch, sorry for the bad nitpick :)

This revision is now accepted and ready to land.Jan 6 2020, 12:38 PM
meven added a comment.Jan 6 2020, 12:39 PM
In D26438#588686, @ivan wrote:

@meven

I hate Qt... defining operator* without operator-> ... ugh

I've approved the patch, sorry for the bad nitpick :)

No worries, thanks for the review

This revision was automatically updated to reflect the committed changes.