Don't create temporary containers just to iterate over them

Stops temporary containers being created needlessly due to API misuse.
For example, it’s common for developers to assume QHash::values() and
QHash::keys() are free and abuse them, failing to realize their
implementation internally actually iterates the whole container, allocates
memory, and fills a new QList.

Added a removeIf generic algorithm, similar to std ones. We can't use std
algorithms with Qt dictionaries because Qt iterators have different
behavior from the std ones.

Found using clazy.

src/app/upgrade.h

@@ -186,9 +186,10 @@ bool upgrade(bool ask = true)
         }
     }
 
-    foreach (const QString &hash, oldResumeData.keys()) {
-        QVariantHash oldTorrent = oldResumeData[hash].toHash();
+    for (auto i = oldResumeData.cbegin(); i != oldResumeData.cend(); ++i) {
+        const QVariantHash oldTorrent = i.value().toHash();
         if (oldTorrent.value("is_magnet", false).toBool()) {
+            const QString &hash = i.key();
             libtorrent::entry resumeData;
             resumeData["qBt-magnetUri"] = oldTorrent.value("magnet_uri").toString().toStdString();
             resumeData["qBt-paused"] = false;

src/base/CMakeLists.txt

@@ -55,6 +55,7 @@ utils/net.h
 utils/random.h
 utils/string.h
 utils/version.h
+algorithm.h
 asyncfilestorage.h
 exceptions.h
 filesystemwatcher.h

src/base/algorithm.h

@@ -0,0 +1,41 @@
+/*
+ * Bittorrent Client using Qt and libtorrent.
+ * Copyright (C) 2018  Vladimir Golovnev <glassez@yandex.ru>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * In addition, as a special exception, the copyright holders give permission to
+ * link this program with the OpenSSL project's "OpenSSL" library (or with
+ * modified versions of it that use the same license as the "OpenSSL" library),
+ * and distribute the linked executables. You must obey the GNU General Public
+ * License in all respects for all of the code used other than "OpenSSL".  If you
+ * modify file(s), you may extend this exception to your version of the file(s),
+ * but you are not obligated to do so. If you do not wish to do so, delete this
+ * exception statement from your version.
+ */
+
+#pragma once
+
+namespace Dict
+{
+    // To be used with QMap, QHash and it's variants
+    template <typename Dictionary, typename BinaryPredicate>
+    void removeIf(Dictionary &&dict, BinaryPredicate p)
+    {
+        auto it = dict.begin();
+        while (it != dict.end())
+            it = (p(it.key(), it.value()) ? dict.erase(it) : it + 1);
+    }
+}

src/base/base.pri

@@ -1,4 +1,5 @@
 HEADERS += \
+    $$PWD/algorithm.h \
     $$PWD/asyncfilestorage.h \
     $$PWD/bittorrent/addtorrentparams.h  \
     $$PWD/bittorrent/cachestatus.h \

src/base/bittorrent/session.cpp

@@ -71,6 +71,7 @@
 #include <libtorrent/session_status.hpp>
 #include <libtorrent/torrent_info.hpp>
 
+#include "base/algorithm.h"
 #include "base/logger.h"
 #include "base/net/downloadhandler.h"
 #include "base/net/downloadmanager.h"
@@ -128,16 +129,16 @@ namespace
     QStringMap map_cast(const QVariantMap &map)
     {
         QStringMap result;
-        foreach (const QString &key, map.keys())
-            result[key] = map.value(key).toString();
+        for (auto i = map.cbegin(); i != map.cend(); ++i)
+            result[i.key()] = i.value().toString();
         return result;
     }
 
     QVariantMap map_cast(const QStringMap &map)
     {
         QVariantMap result;
-        foreach (const QString &key, map.keys())
-            result[key] = map.value(key);
+        for (auto i = map.cbegin(); i != map.cend(); ++i)
+            result[i.key()] = i.value();
         return result;
     }
 
@@ -199,7 +200,8 @@ namespace
     {
         QStringMap expanded = categories;
 
-        foreach (const QString &category, categories.keys()) {
+        for (auto i = categories.cbegin(); i != categories.cend(); ++i) {
+            const QString &category = i.key();
             foreach (const QString &subcat, Session::expandCategory(category)) {
                 if (!expanded.contains(subcat))
                     expanded[subcat] = "";
@@ -785,14 +787,16 @@ bool Session::removeCategory(const QString &name)
     bool result = false;
     if (isSubcategoriesEnabled()) {
         // remove subcategories
-        QString test = name + "/";
-        foreach (const QString &category, m_categories.keys()) {
+        const QString test = name + "/";
+        Dict::removeIf(m_categories, [this, &test, &result](const QString &category, const QString &)
+        {
             if (category.startsWith(test)) {
-                m_categories.remove(category);
                 result = true;
                 emit categoryRemoved(category);
+                return true;
             }
-        }
+            return false;
+        });
     }
 
     result = (m_categories.remove(name) > 0) || result;
@@ -2003,8 +2007,8 @@ void Session::decreaseTorrentsPriority(const QStringList &hashes)
         torrentQueue.pop();
     }
 
-    foreach (const InfoHash &hash, m_loadedMetadata.keys())
-        torrentQueuePositionBottom(m_nativeSession->find_torrent(hash));
+    for (auto i = m_loadedMetadata.cbegin(); i != m_loadedMetadata.cend(); ++i)
+        torrentQueuePositionBottom(m_nativeSession->find_torrent(i.key()));
 }
 
 void Session::topTorrentsPriority(const QStringList &hashes)
@@ -2048,8 +2052,8 @@ void Session::bottomTorrentsPriority(const QStringList &hashes)
         torrentQueue.pop();
     }
 
-    foreach (const InfoHash &hash, m_loadedMetadata.keys())
-        torrentQueuePositionBottom(m_nativeSession->find_torrent(hash));
+    for (auto i = m_loadedMetadata.cbegin(); i != m_loadedMetadata.cend(); ++i)
+        torrentQueuePositionBottom(m_nativeSession->find_torrent(i.key()));
 }
 
 QHash<InfoHash, TorrentHandle *> Session::torrents() const

src/base/filesystemwatcher.cpp

@@ -12,6 +12,7 @@
 #endif
 #endif
 
+#include "algorithm.h"
 #include "base/bittorrent/magneturi.h"
 #include "base/bittorrent/torrentinfo.h"
 #include "base/preferences.h"
@@ -145,22 +146,24 @@ void FileSystemWatcher::processPartialTorrents()
     QStringList noLongerPartial;
 
     // Check which torrents are still partial
-    foreach (const QString &torrentPath, m_partialTorrents.keys()) {
-        if (!QFile::exists(torrentPath)) {
-            m_partialTorrents.remove(torrentPath);
-        }
-        else if (BitTorrent::TorrentInfo::loadFromFile(torrentPath).isValid()) {
+    Dict::removeIf(m_partialTorrents, [&noLongerPartial](const QString &torrentPath, int &value)
+    {
+        if (!QFile::exists(torrentPath))
+            return true;
+
+        if (BitTorrent::TorrentInfo::loadFromFile(torrentPath).isValid()) {
             noLongerPartial << torrentPath;
-            m_partialTorrents.remove(torrentPath);
+            return true;
         }
-        else if (m_partialTorrents[torrentPath] >= MAX_PARTIAL_RETRIES) {
-            m_partialTorrents.remove(torrentPath);
+
+        if (value >= MAX_PARTIAL_RETRIES) {
             QFile::rename(torrentPath, torrentPath + ".invalid");
+            return true;
         }
-        else {
-            ++m_partialTorrents[torrentPath];
-        }
-    }
+
+        ++value;
+        return false;
+    });
 
     // Stop the partial timer if necessary
     if (m_partialTorrents.empty()) {

src/base/net/portforwarder.cpp

@@ -121,8 +121,10 @@ void PortForwarder::start()
     settingsPack.set_bool(libt::settings_pack::enable_natpmp, true);
     m_provider->apply_settings(settingsPack);
 #endif
-    foreach (quint16 port, m_mappedPorts.keys())
-        m_mappedPorts[port] = m_provider->add_port_mapping(libt::session::tcp, port, port);
+    for (auto i = m_mappedPorts.begin(); i != m_mappedPorts.end(); ++i) {
+        // quint16 port = i.key();
+        i.value() = m_provider->add_port_mapping(libt::session::tcp, i.key(), i.key());
+    }
     m_active = true;
     Logger::instance()->addMessage(tr("UPnP / NAT-PMP support [ON]"), Log::INFO);
 }

src/base/search/searchpluginmanager.cpp

@@ -38,6 +38,7 @@
 #include <QPointer>
 #include <QProcess>
 
+#include "base/global.h"
 #include "base/logger.h"
 #include "base/net/downloadmanager.h"
 #include "base/net/downloadhandler.h"
@@ -99,7 +100,7 @@ QStringList SearchPluginManager::allPlugins() const
 QStringList SearchPluginManager::enabledPlugins() const
 {
     QStringList plugins;
-    foreach (const PluginInfo *plugin, m_plugins.values()) {
+    for (const PluginInfo *plugin : qAsConst(m_plugins)) {
         if (plugin->enabled)
             plugins << plugin->name;
     }
@@ -110,7 +111,7 @@ QStringList SearchPluginManager::enabledPlugins() const
 QStringList SearchPluginManager::supportedCategories() const
 {
     QStringList result;
-    foreach (const PluginInfo *plugin, m_plugins.values()) {
+    for (const PluginInfo *plugin : qAsConst(m_plugins)) {
         if (plugin->enabled) {
             foreach (QString cat, plugin->supportedCategories) {
                 if (!result.contains(cat))

src/gui/categoryfiltermodel.cpp

@@ -405,7 +405,8 @@ void CategoryFilterModel::populate()
                                     , [](Torrent *torrent) { return torrent->category().isEmpty(); })));
 
     using Torrent = BitTorrent::TorrentHandle;
-    foreach (const QString &category, session->categories().keys()) {
+    for (auto i = session->categories().cbegin(); i != session->categories().cend(); ++i) {
+        const QString &category = i.key();
         if (m_isSubcategoriesEnabled) {
             CategoryModelItem *parent = m_rootItem;
             foreach (const QString &subcat, session->expandCategory(category)) {

src/gui/categoryfilterwidget.cpp

@@ -232,8 +232,10 @@ void CategoryFilterWidget::removeCategory()
 void CategoryFilterWidget::removeUnusedCategories()
 {
     auto session = BitTorrent::Session::instance();
-    foreach (const QString &category, session->categories().keys())
+    for (auto i = session->categories().cbegin(); i != session->categories().cend(); ++i) {
+        const QString &category = i.key();
         if (model()->data(static_cast<CategoryFilterProxyModel *>(model())->index(category), Qt::UserRole) == 0)
             session->removeCategory(category);
+    }
     updateGeometry();
 }

src/gui/search/pluginselectdlg.cpp

@@ -424,10 +424,10 @@ void PluginSelectDlg::checkForUpdatesFinished(const QHash<QString, PluginVersion
         return;
     }
 
-    foreach (const QString &pluginName, updateInfo.keys()) {
+    for (auto i = updateInfo.cbegin(); i != updateInfo.cend(); ++i) {
         startAsyncOp();
         m_pendingUpdates++;
-        m_pluginManager->updatePlugin(pluginName);
+        m_pluginManager->updatePlugin(i.key());
     }
 }
 

src/gui/transferlistfilterswidget.cpp

@@ -333,7 +333,8 @@ void TrackerFiltersList::setDownloadTrackerFavicon(bool value)
     m_downloadTrackerFavicon = value;
 
     if (m_downloadTrackerFavicon) {
-        foreach (const QString &tracker, m_trackers.keys()) {
+        for (auto i = m_trackers.cbegin(); i != m_trackers.cend(); ++i) {
+            const QString &tracker = i.key();
             if (!tracker.isEmpty())
                 downloadFavicon(QString("http://%1/favicon.ico").arg(tracker));
         }

src/webui/api/appcontroller.cpp

@@ -91,7 +91,7 @@ void AppController::preferencesAction()
     data["incomplete_files_ext"] = session->isAppendExtensionEnabled();
     const QVariantHash dirs = pref->getScanDirs();
     QVariantMap nativeDirs;
-    for (QVariantHash::const_iterator i = dirs.begin(), e = dirs.end(); i != e; ++i) {
+    for (QVariantHash::const_iterator i = dirs.cbegin(), e = dirs.cend(); i != e; ++i) {
         if (i.value().type() == QVariant::Int)
             nativeDirs.insert(Utils::Fs::toNativePath(i.key()), i.value().toInt());
         else
@@ -246,7 +246,7 @@ void AppController::setPreferencesAction()
         QVariantHash oldScanDirs = pref->getScanDirs();
         QVariantHash scanDirs;
         ScanFoldersModel *model = ScanFoldersModel::instance();
-        for (QVariantMap::const_iterator i = nativeDirs.begin(), e = nativeDirs.end(); i != e; ++i) {
+        for (QVariantMap::const_iterator i = nativeDirs.cbegin(), e = nativeDirs.cend(); i != e; ++i) {
             QString folder = Utils::Fs::fromNativePath(i.key());
             int downloadType;
             QString downloadPath;
@@ -275,8 +275,8 @@ void AppController::setPreferencesAction()
         }
 
         // Update deleted folders
-        foreach (QVariant folderVariant, oldScanDirs.keys()) {
-            QString folder = folderVariant.toString();
+        for (auto i = oldScanDirs.cbegin(); i != oldScanDirs.cend(); ++i) {
+            QString folder = i.key();
             if (!scanDirs.contains(folder)) {
                 model->removePath(folder);
                 qDebug("Removed watched folder %s", qUtf8Printable(folder));

src/webui/api/synccontroller.cpp

@@ -152,20 +152,22 @@ namespace
         syncData.clear();
 
         QVariantList removedItems;
-        foreach (QString key, data.keys()) {
+        for (auto i = data.cbegin(); i != data.cend(); ++i) {
+            const QString &key = i.key();
+            const QVariant &value = i.value();
             removedItems.clear();
 
-            switch (static_cast<QMetaType::Type>(data[key].type())) {
+            switch (static_cast<QMetaType::Type>(value.type())) {
             case QMetaType::QVariantMap: {
                     QVariantMap map;
-                    processMap(prevData[key].toMap(), data[key].toMap(), map);
+                    processMap(prevData[key].toMap(), value.toMap(), map);
                     if (!map.isEmpty())
                         syncData[key] = map;
                 }
                 break;
             case QMetaType::QVariantHash: {
                     QVariantMap map;
-                    processHash(prevData[key].toHash(), data[key].toHash(), map, removedItems);
+                    processHash(prevData[key].toHash(), value.toHash(), map, removedItems);
                     if (!map.isEmpty())
                         syncData[key] = map;
                     if (!removedItems.isEmpty())
@@ -174,7 +176,7 @@ namespace
                 break;
             case QMetaType::QVariantList: {
                     QVariantList list;
-                    processList(prevData[key].toList(), data[key].toList(), list, removedItems);
+                    processList(prevData[key].toList(), value.toList(), list, removedItems);
                     if (!list.isEmpty())
                         syncData[key] = list;
                     if (!removedItems.isEmpty())
@@ -190,13 +192,13 @@ namespace
             case QMetaType::ULongLong:
             case QMetaType::UInt:
             case QMetaType::QDateTime:
-                if (prevData[key] != data[key])
-                    syncData[key] = data[key];
+                if (prevData[key] != value)
+                    syncData[key] = value;
                 break;
             default:
                 Q_ASSERT_X(false, "processMap"
                            , QString("Unexpected type: %1")
-                           .arg(QMetaType::typeName(static_cast<QMetaType::Type>(data[key].type())))
+                           .arg(QMetaType::typeName(static_cast<QMetaType::Type>(value.type())))
                            .toUtf8().constData());
             }
         }
@@ -213,25 +215,25 @@ namespace
 
         if (prevData.isEmpty()) {
             // If list was empty before, then difference is a whole new list.
-            foreach (QString key, data.keys())
-                syncData[key] = data[key];
+            for (auto i = data.cbegin(); i != data.cend(); ++i)
+                syncData[i.key()] = i.value();
         }
         else {
-            foreach (QString key, data.keys()) {
-                switch (data[key].type()) {
+            for (auto i = data.cbegin(); i != data.cend(); ++i) {
+                switch (i.value().type()) {
                 case QVariant::Map:
-                    if (!prevData.contains(key)) {
+                    if (!prevData.contains(i.key())) {
                         // new list item found - append it to syncData
-                        syncData[key] = data[key];
+                        syncData[i.key()] = i.value();
                     }
                     else {
                         QVariantMap map;
-                        processMap(prevData[key].toMap(), data[key].toMap(), map);
+                        processMap(prevData[i.key()].toMap(), i.value().toMap(), map);
                         // existing list item found - remove it from prevData
-                        prevData.remove(key);
+                        prevData.remove(i.key());
                         if (!map.isEmpty())
                             // changed list item found - append its changes to syncData
-                            syncData[key] = map;
+                            syncData[i.key()] = map;
                     }
                     break;
                 default:
@@ -242,8 +244,8 @@ namespace
             if (!prevData.isEmpty()) {
                 // prevData contains only items that are missing now -
                 // put them in removedItems
-                foreach (QString s, prevData.keys())
-                    removedItems << s;
+                for (auto i = prevData.cbegin(); i != prevData.cend(); ++i)
+                    removedItems << i.key();
             }
         }
     }
@@ -395,8 +397,8 @@ void SyncController::maindataAction()
     data["torrents"] = torrents;
 
     QVariantList categories;
-    foreach (const QString &category, session->categories().keys())
-        categories << category;
+    for (auto i = session->categories().cbegin(); i != session->categories().cend(); ++i)
+        categories << i.key();
 
     data["categories"] = categories;