Differential D29138

[Solid] Replace foreach (deprecated) with range/index for
ClosedPublic
Actions

Authored by ahmadsamir on Apr 23 2020, 7:16 PM.

Details

Reviewers
dfaure
apol
meven
Group Reviewers
Frameworks
Commits
R245:b9abef40855e: [Solid] Replace foreach (deprecated) with range/index for
Summary

src/imports/devices.cpp
src/solid/devices/backends/fakehw/*
src/solid/devices/backends/fstab/*

Test Plan

make && ctest

Diff Detail

Repository
R245 Solid
Branch
l-foreach (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25846
Build 25864: arc lint + arc unit
ahmadsamir created this revision.Apr 23 2020, 7:16 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 23 2020, 7:16 PM
ahmadsamir requested review of this revision.Apr 23 2020, 7:16 PM
dfaure accepted this revision.Apr 23 2020, 10:21 PM

Wow those iterations over map.keys() were awful.

This revision is now accepted and ready to land.Apr 23 2020, 10:21 PM
bruns added a subscriber: bruns.Apr 23 2020, 10:51 PM
bruns added inline comments.
src/solid/devices/backends/fakehw/fakecdrom.cpp
48

The obviously better solution is to iterate over supported_medialist and swap the key and value of the map.

bruns added inline comments.Apr 23 2020, 10:53 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
38

dito

meven added a comment.Apr 24 2020, 5:43 AM
In D29138#656094, @dfaure wrote:

Wow those iterations over map.keys() were awful.

We still have a few of those in solid :

$ rg ".keys\(\)"
src/tools/solid-hardware/solid-hardware.cpp
151:    Q_FOREACH (const QString &key, properties.keys())

src/solid/devices/backends/udisks2/udisksdevicebackend.cpp
220:    Q_FOREACH (const QString &iface, interfaces_and_properties.keys()) {

src/solid/devices/backends/fstab/fstabhandling.cpp
219:    QStringList devices = globalFstabCache->localData().m_mtabCache.keys();

src/solid/devices/backends/udisks2/udisksmanager.cpp
179:    qCDebug(UDISKS2) << udi << "has new interfaces:" << interfaces_and_properties.keys();
201:    else if (m_deviceCache.contains(udi) && interfaces_and_properties.keys().contains(UD2_DBUS_INTERFACE_FILESYSTEM)) {

src/solid/devices/backends/iokit/iokitstorageaccess.cpp
89:    Q_FOREACH (const QString &property, changes.keys()) {

src/solid/devices/backends/iokit/iokitopticaldrive.cpp
284:    foreach (const Solid::OpticalDrive::MediumType type, d->cdTypeMap.keys()) {
289:    foreach (const Solid::OpticalDrive::MediumType type, d->dvdTypeMap.keys()) {
294:    foreach (const Solid::OpticalDrive::MediumType type, d->bdTypeMap.keys()) {

src/solid/devices/backends/hal/halopticaldisc.cpp
34:    Q_FOREACH (const Solid::OpticalDisc::ContentType type, map.keys()) {

src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
38:    Q_FOREACH (const Solid::OpticalDisc::ContentType type, map.keys()) {

src/solid/devices/backends/fakehw/fakestorageaccess.cpp
60:    Q_FOREACH (const QString &property, changes.keys()) {

src/solid/devices/backends/hal/halcdrom.cpp
55:    Q_FOREACH (const Solid::OpticalDrive::MediumType type, map.keys()) {

src/solid/devices/backends/fakehw/fakecdrom.cpp
48:    Q_FOREACH (const Solid::OpticalDrive::MediumType type, map.keys()) {

src/solid/devices/backends/udisks2/udisksopticaldrive.cpp
63:        Q_FOREACH (const QDBusObjectPath &objPath, reply.value().keys()) {
208:    Q_FOREACH (const Solid::OpticalDrive::MediumType &type, map.keys()) {
src/solid/devices/backends/fakehw/fakemanager.cpp
135

I guess you can const the FakeDevice

147

I guess you can const the FakeDevice

ahmadsamir marked 4 inline comments as done.Apr 24 2020, 2:26 PM
In D29138#656140, @meven wrote:
In D29138#656094, @dfaure wrote:

Wow those iterations over map.keys() were awful.

We still have a few of those in solid :

I know. "foreach" marks the place :)

src/solid/devices/backends/fakehw/fakecdrom.cpp
48

Good point.

ahmadsamir updated this revision to Diff 81098.Apr 24 2020, 2:27 PM
ahmadsamir marked an inline comment as done.

More const
Optimise by iterating over QStringList, and constFind in QMap

bruns added inline comments.Apr 24 2020, 4:18 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

These four lines can be written as
content |= map.value(type, Solid::OpticalDisc::ContentType(0));

ahmadsamir updated this revision to Diff 81121.Apr 24 2020, 5:31 PM
ahmadsamir marked an inline comment as done.

Make the code much shorter with map.value(key, defaultvalue), since the ContentType enum has a NoContent member

ahmadsamir added inline comments.Apr 24 2020, 5:31 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

Nice. :)

I think Solid::OpticalDisc::NoContent is more readable.

bruns added inline comments.Apr 24 2020, 6:49 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

You forgot to remove the old lines.

Can also be applied to supportedMedia

ahmadsamir updated this revision to Diff 81131.Apr 24 2020, 7:21 PM

Remove redundant lines...

ahmadsamir added inline comments.Apr 24 2020, 7:21 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

/* brown paper bag */, removed the lines.

I don't see a default a-la-NoContent MediumType member.

bruns added inline comments.Apr 24 2020, 7:44 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

Just use 0.

ahmadsamir added inline comments.Apr 25 2020, 11:48 AM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

It doesn't make the code more readable; and using QMap::constFind() while iterating is more recognizable/widely-used.

bruns added inline comments.Apr 25 2020, 3:26 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

You are contradicting yourself. You use value() for content, but for supportedMedia it is a bad idea? Also, supportedMedia |= 0 is obviously a noop, while for any named flag one has to lookup the flag value.

ahmadsamir added inline comments.Apr 25 2020, 5:24 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

I was talking about something like Solid::OpticalDrive::MediumType(0); ContentType has Solid::OpticalDisc::NoContent whereas MediumType has nothing similar.

IIUC named casts are preferred over old style casts, so I suggest either we add a MediumType enumerator e.g. NotSupported = 0x00000 or use a static_cast.

bruns added inline comments.Apr 25 2020, 5:26 PM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

Thats not a cast but a constructor, see QFlags documentation

ahmadsamir added inline comments.Apr 27 2020, 9:23 AM
src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
43 ↗(On Diff #81098)

From opticaldrive.h:
Q_DECLARE_FLAGS(MediumTypes, MediumType)

AFAIU, MediumTypes is the QFlags, MediumType is the enum. So Solid::OpticalDrive::MediumType(0) is the enum not the QFlags.

I'll land this and create a separate diff to add an enumerator UnknownMediumType to the MediumType enum, then use it in the for loop.

Closed by commit R245:b9abef40855e: [Solid] Replace foreach (deprecated) with range/index for (authored by ahmadsamir). · Explain WhyApr 27 2020, 9:25 AM
This revision was automatically updated to reflect the committed changes.

Showing Only Differences

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

Revision Contents
Changeset List

Diff 81121

View Options

src/solid/devices/backends/fakehw/fakecdrom.cpp

Loading...
View Options

src/solid/devices/backends/fakehw/fakemanager.cpp

Loading...
View Options

src/solid/devices/backends/fakehw/fakeopticaldisc.cpp

Loading...