ControlObject/ControlPrivateDouble: Various Fixes

src/control/control.cpp

@@ -1,46 +1,38 @@
-#include <QtDebug>
-#include <QSharedPointer>
-
 #include "control/control.h"
 
+#include "control/controlobject.h"
 #include "util/stat.h"
 
-// Static member variable definition
+//static
 UserSettingsPointer ControlDoublePrivate::s_pUserConfig;
 
-QHash<ConfigKey, QWeakPointer<ControlDoublePrivate> > ControlDoublePrivate::s_qCOHash
-GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex);
+//static
+QHash<ConfigKey, QWeakPointer<ControlDoublePrivate>> ControlDoublePrivate::s_qCOHash
+        GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex);
 
+//static
 QHash<ConfigKey, ConfigKey> ControlDoublePrivate::s_qCOAliasHash
-GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex);
+        GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex);
 
+//static
 MMutex ControlDoublePrivate::s_qCOHashMutex;
 
-/*
-ControlDoublePrivate::ControlDoublePrivate()
-        : m_bIgnoreNops(true),
-          m_bTrack(false),
-          m_trackType(Stat::UNSPECIFIED),
-          m_trackFlags(Stat::COUNT | Stat::SUM | Stat::AVERAGE |
-                       Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX),
-          m_confirmRequired(false) {
-    initialize();
-}
-*/
-
-ControlDoublePrivate::ControlDoublePrivate(ConfigKey key,
-                                           ControlObject* pCreatorCO,
-                                           bool bIgnoreNops, bool bTrack,
-                                           bool bPersist, double defaultValue)
+ControlDoublePrivate::ControlDoublePrivate(
+        ConfigKey key,
+        ControlObject* pCreatorCO,
+        bool bIgnoreNops,
+        bool bTrack,
+        bool bPersist,
+        double defaultValue)
         : m_key(key),
+          m_pCreatorCO(pCreatorCO),
           m_bPersistInConfiguration(bPersist),
           m_bIgnoreNops(bIgnoreNops),
           m_bTrack(bTrack),
           m_trackType(Stat::UNSPECIFIED),
           m_trackFlags(Stat::COUNT | Stat::SUM | Stat::AVERAGE |
-                       Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX),
-          m_confirmRequired(false),
-          m_pCreatorCO(pCreatorCO) {
+                  Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX),
+          m_confirmRequired(false) {
     initialize(defaultValue);
 }
 
@@ -112,59 +104,88 @@ QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(
     VERIFY_OR_DEBUG_ASSERT(!key.isEmpty()) {
         qWarning() << "ControlDoublePrivate::getControl returning NULL"
                    << "for empty ConfigKey.";
-        return QSharedPointer<ControlDoublePrivate>();
+        return nullptr;
     }
 
-    QSharedPointer<ControlDoublePrivate> pControl;
     // Scope for MMutexLocker.
     {
-        MMutexLocker locker(&s_qCOHashMutex);
-        auto it = s_qCOHash.constFind(key);
-        if (it != s_qCOHash.constEnd()) {
-            if (pCreatorCO) {
-                qWarning() << "ControlObject" << key.group << key.item << "already created";
-                DEBUG_ASSERT(!"ControlObject already created");
+        const MMutexLocker locker(&s_qCOHashMutex);
+        const auto it = s_qCOHash.find(key);
+        if (it != s_qCOHash.end()) {
+            auto pControl = it.value().lock();
+            if (pControl) {
+                // Control object already exists
+                VERIFY_OR_DEBUG_ASSERT(!pCreatorCO) {
+                    qWarning()
+                            << "ControlObject"
+                            << key.group << key.item
+                            << "already created";
+                    return nullptr;
+                }
+                return pControl;
             } else {
-                pControl = it.value();
+                // The weak pointer has become invalid and can be cleaned up
+                s_qCOHash.erase(it);
             }
         }
     }
 
-    if (pControl == NULL) {
-        if (pCreatorCO) {
-            pControl = QSharedPointer<ControlDoublePrivate>(
-                    new ControlDoublePrivate(key, pCreatorCO, bIgnoreNops,
-                                             bTrack, bPersist, defaultValue));
-            MMutexLocker locker(&s_qCOHashMutex);
-            //qDebug() << "ControlDoublePrivate::s_qCOHash.insert(" << key.group << "," << key.item << ")";
-            s_qCOHash.insert(key, pControl);
-        } else if (!flags.testFlag(ControlFlag::NoWarnIfMissing)) {
-            qWarning() << "ControlDoublePrivate::getControl returning NULL for ("
-                       << key.group << "," << key.item << ")";
-            DEBUG_ASSERT(flags.testFlag(ControlFlag::NoAssertIfMissing));
-        }
+    if (pCreatorCO) {
+        auto pControl = QSharedPointer<ControlDoublePrivate>(
+                new ControlDoublePrivate(key,
+                        pCreatorCO,
+                        bIgnoreNops,
+                        bTrack,
+                        bPersist,
+                        defaultValue));
+        const MMutexLocker locker(&s_qCOHashMutex);
+        //qDebug() << "ControlDoublePrivate::s_qCOHash.insert(" << key.group << "," << key.item << ")";
+        s_qCOHash.insert(key, pControl);
+        return pControl;
+    }
+
+    if (!flags.testFlag(ControlFlag::NoWarnIfMissing)) {
+        qWarning() << "ControlDoublePrivate::getControl returning NULL for ("
+                   << key.group << "," << key.item << ")";
+        DEBUG_ASSERT(flags.testFlag(ControlFlag::NoAssertIfMissing));
     }
-    return pControl;
+    return nullptr;
 }
 
 // static
-void ControlDoublePrivate::getControls(
-        QList<QSharedPointer<ControlDoublePrivate> >* pControlList) {
-    s_qCOHashMutex.lock();
-    pControlList->clear();
-    for (auto it = s_qCOHash.constBegin(); it != s_qCOHash.constEnd(); ++it) {
-        QSharedPointer<ControlDoublePrivate> pControl = it.value();
-        if (!pControl.isNull()) {
-            pControlList->push_back(pControl);
+QList<QSharedPointer<ControlDoublePrivate>> ControlDoublePrivate::getAllInstances() {
+    QList<QSharedPointer<ControlDoublePrivate>> result;
+    MMutexLocker locker(&s_qCOHashMutex);
+    result.reserve(s_qCOHash.size());
+    for (auto it = s_qCOHash.begin(); it != s_qCOHash.end(); ++it) {
+        auto pControl = it.value().lock();
+        if (pControl) {
+            result.append(std::move(pControl));
+        } else {
+            // The weak pointer has become invalid and can be cleaned up
+            s_qCOHash.erase(it);
         }
     }
-    s_qCOHashMutex.unlock();
+    return result;
 }
 
 // static
-QHash<ConfigKey, ConfigKey> ControlDoublePrivate::getControlAliases() {
+QList<QSharedPointer<ControlDoublePrivate>> ControlDoublePrivate::takeAllInstances() {
+    QList<QSharedPointer<ControlDoublePrivate>> result;
     MMutexLocker locker(&s_qCOHashMutex);
-    return s_qCOAliasHash;
+    result.reserve(s_qCOHash.size());
+    for (auto it = s_qCOHash.begin(); it != s_qCOHash.end(); ++it) {
+        auto pControl = it.value().lock();
+        if (pControl) {
+            result.append(std::move(pControl));
+        }
+    }
+    s_qCOHash.clear();
+    return result;
+}
+
+void ControlDoublePrivate::deleteCreatorCO() {
+    delete m_pCreatorCO.fetchAndStoreOrdered(nullptr);
 }
 
 void ControlDoublePrivate::reset() {

src/control/control.h

@@ -1,10 +1,10 @@
-#ifndef CONTROL_H
-#define CONTROL_H
+#pragma once
 
+#include <QAtomicPointer>
 #include <QHash>
-#include <QString>
 #include <QObject>
-#include <QAtomicPointer>
+#include <QSharedPointer>
+#include <QString>
 
 #include "control/controlbehavior.h"
 #include "control/controlvalue.h"
@@ -26,7 +26,7 @@ Q_DECLARE_OPERATORS_FOR_FLAGS(ControlFlags)
 class ControlDoublePrivate : public QObject {
     Q_OBJECT
   public:
-    virtual ~ControlDoublePrivate();
+    ~ControlDoublePrivate() override;
 
     // Used to implement control persistence. All controls that are marked
     // "persist in user config" get and set their value on creation/deletion
@@ -43,18 +43,24 @@ class ControlDoublePrivate : public QObject {
     // Gets the ControlDoublePrivate matching the given ConfigKey. If pCreatorCO
     // is non-NULL, allocates a new ControlDoublePrivate for the ConfigKey if
     // one does not exist.
-    static QSharedPointer<ControlDoublePrivate> getControl(const ConfigKey& key,
+    static QSharedPointer<ControlDoublePrivate> getControl(
+            const ConfigKey& key,
             ControlFlags flags = ControlFlag::None,
-            ControlObject* pCreatorCO = NULL,
+            ControlObject* pCreatorCO = nullptr,
             bool bIgnoreNops = true,
             bool bTrack = false,
             bool bPersist = false,
             double defaultValue = 0.0);
 
-    // Adds all ControlDoublePrivate that currently exist to pControlList
-    static void getControls(QList<QSharedPointer<ControlDoublePrivate> >* pControlsList);
+    // Returns a list of all existing instances.
+    static QList<QSharedPointer<ControlDoublePrivate>> getAllInstances();
+    // Clears all existing instances and returns them as a list.
+    static QList<QSharedPointer<ControlDoublePrivate>> takeAllInstances();
 
-    static QHash<ConfigKey, ConfigKey> getControlAliases();
+    static QHash<ConfigKey, ConfigKey> getControlAliases() {
+        // Implicitly shared classes can safely be copied across threads
+        return s_qCOAliasHash;
+    }
 
     const QString& name() const {
         return m_name;
@@ -78,16 +84,18 @@ class ControlDoublePrivate : public QObject {
     // ValueChangeRequest slot.
     void setAndConfirm(double value, QObject* pSender);
     // Gets the control value.
-    inline double get() const {
+    double get() const {
         return m_value.getValue();
     }
     // Resets the control value to its default.
     void reset();
 
     // Set the behavior to be used when setting values and translating between
     // parameter and value space. Returns the previously set behavior (if any).
-    // The caller must not delete the behavior at any time. The memory is managed
-    // by this function.
+    // Callers must allocate the passed behavior using new and ownership to this
+    // memory is passed with the function call!!
+    // TODO: Pass a std::unique_ptr instead of a plain pointer to ensure this
+    // transfer of ownership.
     void setBehavior(ControlNumericBehavior* pBehavior);
 
     void setParameter(double dParam, QObject* pSender);
@@ -98,27 +106,28 @@ class ControlDoublePrivate : public QObject {
     void setValueFromMidi(MidiOpCode opcode, double dParam);
     double getMidiParameter() const;
 
-    inline bool ignoreNops() const {
+    bool ignoreNops() const {
         return m_bIgnoreNops;
     }
 
-    inline void setDefaultValue(double dValue) {
+    void setDefaultValue(double dValue) {
         m_defaultValue.setValue(dValue);
     }
 
-    inline double defaultValue() const {
+    double defaultValue() const {
         return m_defaultValue.getValue();
     }
 
-    inline ControlObject* getCreatorCO() const {
-        return m_pCreatorCO;
+    ControlObject* getCreatorCO() const {
+        return m_pCreatorCO.loadAcquire();
     }
 
-    inline void removeCreatorCO() {
-        m_pCreatorCO = NULL;
+    bool resetCreatorCO(ControlObject* pCreatorCO) {
+        return m_pCreatorCO.testAndSetOrdered(pCreatorCO, nullptr);
     }
+    void deleteCreatorCO();
 
-    inline ConfigKey getKey() {
+    ConfigKey getKey() {
         return m_key;
     }
 
@@ -144,25 +153,30 @@ class ControlDoublePrivate : public QObject {
     void valueChangeRequest(double value);
 
   private:
-    ControlDoublePrivate(ConfigKey key, ControlObject* pCreatorCO,
-                         bool bIgnoreNops, bool bTrack, bool bPersist,
-                         double defaultValue);
+    ControlDoublePrivate(
+            ConfigKey key,
+            ControlObject* pCreatorCO,
+            bool bIgnoreNops,
+            bool bTrack,
+            bool bPersist,
+            double defaultValue);
+    ControlDoublePrivate(ControlDoublePrivate&&) = delete;
+    ControlDoublePrivate(const ControlDoublePrivate&) = delete;
+    ControlDoublePrivate& operator=(ControlDoublePrivate&&) = delete;
+    ControlDoublePrivate& operator=(const ControlDoublePrivate&) = delete;
+
     void initialize(double defaultValue);
     void setInner(double value, QObject* pSender);
 
-    ConfigKey m_key;
+    const ConfigKey m_key;
+
+    QAtomicPointer<ControlObject> m_pCreatorCO;
 
     // Whether the control should persist in the Mixxx user configuration. The
     // value is loaded from configuration when the control is created and
     // written to the configuration when the control is deleted.
     bool m_bPersistInConfiguration;
 
-    // User-visible, i18n name for what the control is.
-    QString m_name;
-
-    // User-visible, i18n description for what the control does.
-    QString m_description;
-
     // Whether to ignore sets which would have no effect.
     bool m_bIgnoreNops;
 
@@ -173,15 +187,19 @@ class ControlDoublePrivate : public QObject {
     int m_trackFlags;
     bool m_confirmRequired;
 
+    // User-visible, i18n name for what the control is.
+    QString m_name;
+
+    // User-visible, i18n description for what the control does.
+    QString m_description;
+
     // The control value.
     ControlValueAtomic<double> m_value;
     // The default control value.
     ControlValueAtomic<double> m_defaultValue;
 
     QSharedPointer<ControlNumericBehavior> m_pBehavior;
 
-    ControlObject* m_pCreatorCO;
-
     // Hack to implement persistent controls. This is a pointer to the current
     // user configuration object (if one exists). In general, we do not want the
     // user configuration to be a singleton -- objects that need access to it
@@ -191,14 +209,12 @@ class ControlDoublePrivate : public QObject {
     static UserSettingsPointer s_pUserConfig;
 
     // Hash of ControlDoublePrivate instantiations.
-    static QHash<ConfigKey, QWeakPointer<ControlDoublePrivate> > s_qCOHash;
+    static QHash<ConfigKey, QWeakPointer<ControlDoublePrivate>> s_qCOHash;
+
     // Hash of aliases between ConfigKeys. Solely used for looking up the first
     // alias associated with a key.
     static QHash<ConfigKey, ConfigKey> s_qCOAliasHash;
 
     // Mutex guarding access to s_qCOHash and s_qCOAliasHash.
     static MMutex s_qCOHashMutex;
 };
-
-
-#endif /* CONTROL_H */

src/control/controlobject.cpp

@@ -54,7 +54,9 @@ ControlObject::ControlObject(ConfigKey key, bool bIgnoreNops, bool bTrack,
 
 ControlObject::~ControlObject() {
     if (m_pControl) {
-        m_pControl->removeCreatorCO();
+        const bool success = m_pControl->resetCreatorCO(this);
+        Q_UNUSED(success);
+        DEBUG_ASSERT(success);
     }
 }