From ac42ccb5e4d9662e69acc3aacd95f9731ec5aa09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Pereira?= Date: Tue, 6 Mar 2018 14:50:10 +0000 Subject: [PATCH] Don't create temporary containers just to iterate over them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 5 +-- src/base/CMakeLists.txt | 1 + src/base/algorithm.h | 41 +++++++++++++++++++++++ src/base/base.pri | 1 + src/base/bittorrent/session.cpp | 30 +++++++++-------- src/base/filesystemwatcher.cpp | 27 ++++++++------- src/base/net/portforwarder.cpp | 6 ++-- src/base/search/searchpluginmanager.cpp | 5 +-- src/gui/categoryfiltermodel.cpp | 3 +- src/gui/categoryfilterwidget.cpp | 4 ++- src/gui/search/pluginselectdlg.cpp | 4 +-- src/gui/transferlistfilterswidget.cpp | 3 +- src/webui/api/appcontroller.cpp | 8 ++--- src/webui/api/synccontroller.cpp | 44 +++++++++++++------------ 14 files changed, 121 insertions(+), 61 deletions(-) create mode 100644 src/base/algorithm.h diff --git a/src/app/upgrade.h b/src/app/upgrade.h index 2a591ed4a..69edcda67 100644 --- a/src/app/upgrade.h +++ b/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; diff --git a/src/base/CMakeLists.txt b/src/base/CMakeLists.txt index 17d01fb0b..a9cdb45a5 100644 --- a/src/base/CMakeLists.txt +++ b/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 diff --git a/src/base/algorithm.h b/src/base/algorithm.h new file mode 100644 index 000000000..0d2f95679 --- /dev/null +++ b/src/base/algorithm.h @@ -0,0 +1,41 @@ +/* + * Bittorrent Client using Qt and libtorrent. + * Copyright (C) 2018 Vladimir Golovnev + * + * 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 + 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); + } +} diff --git a/src/base/base.pri b/src/base/base.pri index c59ae21bc..c84c49c93 100644 --- a/src/base/base.pri +++ b/src/base/base.pri @@ -1,4 +1,5 @@ HEADERS += \ + $$PWD/algorithm.h \ $$PWD/asyncfilestorage.h \ $$PWD/bittorrent/addtorrentparams.h \ $$PWD/bittorrent/cachestatus.h \ diff --git a/src/base/bittorrent/session.cpp b/src/base/bittorrent/session.cpp index d9d0e5879..4894e530a 100644 --- a/src/base/bittorrent/session.cpp +++ b/src/base/bittorrent/session.cpp @@ -71,6 +71,7 @@ #include #include +#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 Session::torrents() const diff --git a/src/base/filesystemwatcher.cpp b/src/base/filesystemwatcher.cpp index 6e387cee9..33aef074a 100644 --- a/src/base/filesystemwatcher.cpp +++ b/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()) { diff --git a/src/base/net/portforwarder.cpp b/src/base/net/portforwarder.cpp index aaa89d3f0..44d350d29 100644 --- a/src/base/net/portforwarder.cpp +++ b/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); } diff --git a/src/base/search/searchpluginmanager.cpp b/src/base/search/searchpluginmanager.cpp index 9849a2164..edf4804e7 100644 --- a/src/base/search/searchpluginmanager.cpp +++ b/src/base/search/searchpluginmanager.cpp @@ -38,6 +38,7 @@ #include #include +#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)) diff --git a/src/gui/categoryfiltermodel.cpp b/src/gui/categoryfiltermodel.cpp index d2626535e..23dcd5454 100644 --- a/src/gui/categoryfiltermodel.cpp +++ b/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)) { diff --git a/src/gui/categoryfilterwidget.cpp b/src/gui/categoryfilterwidget.cpp index efba70854..3fcd71e53 100644 --- a/src/gui/categoryfilterwidget.cpp +++ b/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(model())->index(category), Qt::UserRole) == 0) session->removeCategory(category); + } updateGeometry(); } diff --git a/src/gui/search/pluginselectdlg.cpp b/src/gui/search/pluginselectdlg.cpp index 46b2d943b..f1756dc50 100644 --- a/src/gui/search/pluginselectdlg.cpp +++ b/src/gui/search/pluginselectdlg.cpp @@ -424,10 +424,10 @@ void PluginSelectDlg::checkForUpdatesFinished(const QHashupdatePlugin(pluginName); + m_pluginManager->updatePlugin(i.key()); } } diff --git a/src/gui/transferlistfilterswidget.cpp b/src/gui/transferlistfilterswidget.cpp index b11db0a2c..f551c2511 100644 --- a/src/gui/transferlistfilterswidget.cpp +++ b/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)); } diff --git a/src/webui/api/appcontroller.cpp b/src/webui/api/appcontroller.cpp index c58b27105..6da296513 100644 --- a/src/webui/api/appcontroller.cpp +++ b/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)); diff --git a/src/webui/api/synccontroller.cpp b/src/webui/api/synccontroller.cpp index 570e721e7..f88fbd51a 100644 --- a/src/webui/api/synccontroller.cpp +++ b/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(data[key].type())) { + switch (static_cast(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(data[key].type()))) + .arg(QMetaType::typeName(static_cast(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;