PluginManager: Code clean up

Mostly refactor some explicit loops, and nicer reverse looping.

Change-Id: I102b86da597b37cd496762bd776af73ec407d838
Reviewed-by: Tobias Hunger <tobias.hunger@qt.io>

src/libs/extensionsystem/pluginmanager.cpp

@@ -369,12 +369,10 @@ void PluginManager::loadPlugins()
 */
 bool PluginManager::hasError()
 {
-    foreach (PluginSpec *spec, plugins()) {
+    return Utils::anyOf(plugins(), [](PluginSpec *spec) {
         // only show errors on startup if plugin is enabled.
-        if (spec->hasError() && spec->isEffectivelyEnabled())
+        return spec->hasError() && spec->isEffectivelyEnabled();
-            return true;
+    });
-    }
-    return false;
 }
 
 /*!
@@ -382,19 +380,11 @@ bool PluginManager::hasError()
  */
 QSet<PluginSpec *> PluginManager::pluginsRequiringPlugin(PluginSpec *spec)
 {
-    QSet<PluginSpec *> dependingPlugins;
+    QSet<PluginSpec *> dependingPlugins({spec});
-    dependingPlugins.insert(spec);
+    // recursively add plugins that depend on plugins that.... that depend on spec
-    foreach (PluginSpec *checkSpec, d->loadQueue()) {
+    foreach (PluginSpec *spec, d->loadQueue()) {
-        QHashIterator<PluginDependency, PluginSpec *> depIt(checkSpec->dependencySpecs());
+        if (spec->requiresAny(dependingPlugins))
-        while (depIt.hasNext()) {
+            dependingPlugins.insert(spec);
-            depIt.next();
-            if (depIt.key().type != PluginDependency::Required)
-                continue;
-            if (dependingPlugins.contains(depIt.value())) {
-                dependingPlugins.insert(checkSpec);
-                break; // no use to check other dependencies, continue with load queue
-            }
-        }
     }
     dependingPlugins.remove(spec);
     return dependingPlugins;
@@ -665,9 +655,7 @@ bool PluginManager::parseOptions(const QStringList &args,
 
 static inline void indent(QTextStream &str, int indent)
 {
-    const QChar blank = QLatin1Char(' ');
+    str << QString(indent, ' ');
-    for (int i = 0 ; i < indent; i++)
-        str << blank;
 }
 
 static inline void formatOption(QTextStream &str,
@@ -927,12 +915,9 @@ void PluginManagerPrivate::stopAll()
 */
 void PluginManagerPrivate::deleteAll()
 {
-    QList<PluginSpec *> queue = loadQueue();
+    Utils::reverseForeach(loadQueue(), [this](PluginSpec *spec) {
-    QListIterator<PluginSpec *> it(queue);
+        loadPlugin(spec, PluginSpec::Deleted);
-    it.toBack();
+    });
-    while (it.hasPrevious()) {
-        loadPlugin(it.previous(), PluginSpec::Deleted);
-    }
 }
 
 #ifdef WITH_TESTS
@@ -1225,10 +1210,7 @@ void PluginManagerPrivate::loadPlugins()
     foreach (PluginSpec *spec, queue) {
         loadPlugin(spec, PluginSpec::Initialized);
     }
-    QListIterator<PluginSpec *> it(queue);
+    Utils::reverseForeach(queue, [this](PluginSpec *spec) {
-    it.toBack();
-    while (it.hasPrevious()) {
-        PluginSpec *spec = it.previous();
         loadPlugin(spec, PluginSpec::Running);
         if (spec->state() == PluginSpec::Running) {
             delayedInitializeQueue.append(spec);
@@ -1236,7 +1218,7 @@ void PluginManagerPrivate::loadPlugins()
             // Plugin initialization failed, so cleanup after it
             spec->d->kill();
         }
-    }
+    });
     emit q->pluginsChanged();
 
     delayedInitializeTimer = new QTimer;
@@ -1420,6 +1402,21 @@ void PluginManagerPrivate::setPluginPaths(const QStringList &paths)
     readPluginPaths();
 }
 
+static QStringList pluginFiles(const QStringList &pluginPaths)
+{
+    QStringList pluginFiles;
+    QStringList searchPaths = pluginPaths;
+    while (!searchPaths.isEmpty()) {
+        const QDir dir(searchPaths.takeFirst());
+        const QFileInfoList files = dir.entryInfoList(QDir::Files | QDir::NoSymLinks);
+        const QStringList absoluteFilePaths = Utils::transform(files, &QFileInfo::absoluteFilePath);
+        pluginFiles += Utils::filtered(absoluteFilePaths, [](const QString &path) { return QLibrary::isLibrary(path); });
+        const QFileInfoList dirs = dir.entryInfoList(QDir::Dirs|QDir::NoDotAndDotDot);
+        searchPaths += Utils::transform(dirs, &QFileInfo::absoluteFilePath);
+    }
+    return pluginFiles;
+}
+
 /*!
     \internal
 */
@@ -1430,24 +1427,10 @@ void PluginManagerPrivate::readPluginPaths()
     pluginSpecs.clear();
     pluginCategories.clear();
 
-    QStringList pluginFiles;
+    auto defaultCollection = new PluginCollection(QString());
-    QStringList searchPaths = pluginPaths;
-    while (!searchPaths.isEmpty()) {
-        const QDir dir(searchPaths.takeFirst());
-        const QFileInfoList files = dir.entryInfoList(QDir::Files | QDir::NoSymLinks);
-        foreach (const QFileInfo &file, files) {
-            const QString filePath = file.absoluteFilePath();
-            if (QLibrary::isLibrary(filePath))
-                pluginFiles.append(filePath);
-        }
-        const QFileInfoList dirs = dir.entryInfoList(QDir::Dirs|QDir::NoDotAndDotDot);
-        foreach (const QFileInfo &subdir, dirs)
-            searchPaths << subdir.absoluteFilePath();
-    }
-    defaultCollection = new PluginCollection(QString());
     pluginCategories.insert(QString(), defaultCollection);
 
-    foreach (const QString &pluginFile, pluginFiles) {
+    foreach (const QString &pluginFile, pluginFiles(pluginPaths)) {
         PluginSpec *spec = new PluginSpec;
         if (!spec->d->read(pluginFile)) { // not a Qt Creator plugin
             delete spec;
@@ -1492,12 +1475,9 @@ void PluginManagerPrivate::resolveDependencies()
         spec->d->resolveDependencies(pluginSpecs);
     }
 
-    QListIterator<PluginSpec *> it(loadQueue());
+    Utils::reverseForeach(loadQueue(), [](PluginSpec *spec) {
-    it.toBack();
-    while (it.hasPrevious()) {
-        PluginSpec *spec = it.previous();
         spec->d->enableDependenciesIndirectly();
-    }
+    });
 }
 
 void PluginManagerPrivate::enableOnlyTestedSpecs()
@@ -1529,20 +1509,19 @@ void PluginManagerPrivate::enableOnlyTestedSpecs()
     }
 }
 
- // Look in argument descriptions of the specs for the option.
+// Look in argument descriptions of the specs for the option.
 PluginSpec *PluginManagerPrivate::pluginForOption(const QString &option, bool *requiresArgument) const
 {
     // Look in the plugins for an option
     *requiresArgument = false;
-    foreach (PluginSpec *ps, pluginSpecs) {
+    foreach (PluginSpec *spec, pluginSpecs) {
-        const PluginSpec::PluginArgumentDescriptions pargs = ps->argumentDescriptions();
+        PluginArgumentDescription match = Utils::findOrDefault(spec->argumentDescriptions(),
-        if (!pargs.empty()) {
+                                                               [option](PluginArgumentDescription pad) {
-            foreach (PluginArgumentDescription pad, pargs) {
+                                                                   return pad.name == option;
-                if (pad.name == option) {
+                                                               });
-                    *requiresArgument = !pad.parameter.isEmpty();
+        if (!match.name.isEmpty()) {
-                    return ps;
+            *requiresArgument = !match.parameter.isEmpty();
-                }
+            return spec;
-            }
         }
     }
     return 0;
@@ -1550,10 +1529,7 @@ PluginSpec *PluginManagerPrivate::pluginForOption(const QString &option, bool *r
 
 PluginSpec *PluginManagerPrivate::pluginByName(const QString &name) const
 {
-    foreach (PluginSpec *spec, pluginSpecs)
+    return Utils::findOrDefault(pluginSpecs, [name](PluginSpec *spec) { return spec->name() == name; });
-        if (spec->name() == name)
-            return spec;
-    return 0;
 }
 
 void PluginManagerPrivate::initProfiling()
@@ -1586,22 +1562,19 @@ void PluginManagerPrivate::profilingReport(const char *what, const PluginSpec *s
 void PluginManagerPrivate::profilingSummary() const
 {
     if (!m_profileTimer.isNull()) {
-        typedef QMultiMap<int, const PluginSpec *> Sorter;
+        QMultiMap<int, const PluginSpec *> sorter;
-        Sorter sorter;
         int total = 0;
 
-        QHash<const PluginSpec *, int>::ConstIterator it1 = m_profileTotal.constBegin();
+        auto totalEnd = m_profileTotal.constEnd();
-        QHash<const PluginSpec *, int>::ConstIterator et1 = m_profileTotal.constEnd();
+        for (auto it = m_profileTotal.constBegin(); it != totalEnd; ++it) {
-        for (; it1 != et1; ++it1) {
+            sorter.insert(it.value(), it.key());
-            sorter.insert(it1.value(), it1.key());
+            total += it.value();
-            total += it1.value();
         }
 
-        Sorter::ConstIterator it2 = sorter.constBegin();
+        auto sorterEnd = sorter.constEnd();
-        Sorter::ConstIterator et2 = sorter.constEnd();
+        for (auto it = sorter.constBegin(); it != sorterEnd; ++it)
-        for (; it2 != et2; ++it2)
+            qDebug("%-22s %8dms   ( %5.2f%% )", qPrintable(it.value()->name()),
-            qDebug("%-22s %8dms   ( %5.2f%% )", qPrintable(it2.value()->name()),
+                it.key(), 100.0 * it.key() / total);
-                it2.key(), 100.0 * it2.key() / total);
          qDebug("Total: %8dms", total);
     }
 }
@@ -1693,12 +1666,9 @@ bool PluginManager::isInitializationDone()
 QObject *PluginManager::getObjectByName(const QString &name)
 {
     QReadLocker lock(&d->m_lock);
-    QList<QObject *> all = allObjects();
+    return Utils::findOrDefault(allObjects(), [&name](const QObject *obj) {
-    foreach (QObject *obj, all) {
+        return obj->objectName() == name;
-        if (obj->objectName() == name)
+    });
-            return obj;
-    }
-    return 0;
 }
 
 /*!
@@ -1711,10 +1681,7 @@ QObject *PluginManager::getObjectByClassName(const QString &className)
 {
     const QByteArray ba = className.toUtf8();
     QReadLocker lock(&d->m_lock);
-    QList<QObject *> all = allObjects();
+    return Utils::findOrDefault(allObjects(), [&ba](const QObject *obj) {
-    foreach (QObject *obj, all) {
+        return obj->inherits(ba.constData());
-        if (obj->inherits(ba.constData()))
+    });
-            return obj;
-    }
-    return 0;
 }

src/libs/extensionsystem/pluginmanager_p.h

@@ -134,7 +134,6 @@ public:
     bool m_isInitializationDone = false;
 
 private:
-    PluginCollection *defaultCollection;
     PluginManager *q;
 
     void nextDelayedInitialize();

src/libs/extensionsystem/pluginspec.cpp

@@ -30,6 +30,8 @@
 #include "iplugin_p.h"
 #include "pluginmanager.h"
 
+#include <utils/algorithm.h>
+
 #include <QCoreApplication>
 #include <QDebug>
 #include <QDir>
@@ -464,6 +466,14 @@ QHash<PluginDependency, PluginSpec *> PluginSpec::dependencySpecs() const
     return d->dependencySpecs;
 }
 
+bool PluginSpec::requiresAny(const QSet<PluginSpec *> &plugins) const
+{
+    return Utils::anyOf(d->dependencySpecs.keys(), [this, &plugins](const PluginDependency &dep) {
+        return dep.type == PluginDependency::Required
+               && plugins.contains(d->dependencySpecs.value(dep));
+    });
+}
+
 //==========PluginSpecPrivate==================
 
 namespace {
@@ -895,14 +905,9 @@ bool PluginSpecPrivate::resolveDependencies(const QList<PluginSpec *> &specs)
     }
     QHash<PluginDependency, PluginSpec *> resolvedDependencies;
     foreach (const PluginDependency &dependency, dependencies) {
-        PluginSpec *found = 0;
+        PluginSpec * const found = Utils::findOrDefault(specs, [&dependency](PluginSpec *spec) {
-
+            return spec->provides(dependency.name, dependency.version);
-        foreach (PluginSpec *spec, specs) {
+        });
-            if (spec->provides(dependency.name, dependency.version)) {
-                found = spec;
-                break;
-            }
-        }
         if (!found) {
             if (dependency.type == PluginDependency::Required) {
                 hasError = true;

src/libs/extensionsystem/pluginspec.h

@@ -119,6 +119,7 @@ public:
 
     // dependency specs, valid after 'Resolved' state is reached
     QHash<PluginDependency, PluginSpec *> dependencySpecs() const;
+    bool requiresAny(const QSet<PluginSpec *> &plugins) const;
 
     // linked plugin instance, valid after 'Loaded' state is reached
     IPlugin *plugin() const;

src/libs/utils/algorithm.h

@@ -418,4 +418,15 @@ inline void sort(Container &c, Predicate p)
     std::sort(c.begin(), c.end(), p);
 }
 
+//////////////////
+// reverseForeach
+/////////////////
+template <typename Container, typename Op>
+inline void reverseForeach(const Container &c, const Op &operation)
+{
+    auto rend = c.rend();
+    for (auto it = c.rbegin(); it != rend; ++it)
+        operation(*it);
+}
+
 }

tests/auto/extensionsystem/pluginmanager/tst_pluginmanager.cpp

@@ -140,8 +140,10 @@ void tst_PluginManager::getObject()
     object2b->setObjectName(objectName);
     m_pm->addObject(object2);
     QCOMPARE(m_pm->getObject<MyClass11>(), static_cast<MyClass11 *>(0));
+    QCOMPARE(m_pm->getObjectByClassName("MyClass11"), static_cast<QObject*>(0));
     QCOMPARE(m_pm->getObject<MyClass1>(), static_cast<MyClass1 *>(0));
     QCOMPARE(m_pm->getObject<MyClass2>(), object2);
+    QCOMPARE(m_pm->getObjectByClassName("MyClass2"), object2);
     m_pm->addObject(object11);
     QCOMPARE(m_pm->getObject<MyClass11>(), object11);
     QCOMPARE(m_pm->getObject<MyClass1>(), qobject_cast<MyClass1 *>(object11));