From 673ded0d543cbdc2cf6e746b6bee7c1d21af8f90 Mon Sep 17 00:00:00 2001 From: Manuel Nickschas Date: Wed, 27 May 2020 23:21:20 +0200 Subject: [PATCH] common: Make SyncableObject non-copyable QObjects are non-copyable for good reasons. SyncableObject, although deriving from QObject, was made sort-of copyable by adding custom copy ctor and copy assignment operator that did not make a full copy of the underlying QObject, but just copied (most) properties. Several classes deriving from SyncableObject then implemented their own versions of those special member functions on top of that. This was not only rather unexpected and intransparent behavior due to the incomplete copy functionality, but also a most fragile hack since one had to remember to update those functions when adding or modifying properties. In addition, newer compilers apply a somewhat stricter interpretation of the C++ standard, basically enforcing the Rule of Three by omitting implicit generation of copy ctor and/or copy assignment operating in case certain user-defined special member functions exist. In particular, Clang 10 started to emit several warnings (and we already worked around a similar warning from GCC 9+ in cc21148). Instead of adding more workarounds further obfuscating matters, remove the relevant special member functions altogether and thus make SyncableObject and its derivatives non-copyable as they should be. Modify affected code to cope with this cleanly, and without abusing unexpected behavior. --- src/common/aliasmanager.cpp | 10 ------ src/common/aliasmanager.h | 1 - src/common/highlightrulemanager.cpp | 12 ------- src/common/highlightrulemanager.h | 1 - src/common/ignorelistmanager.cpp | 10 ------ src/common/ignorelistmanager.h | 1 - src/common/syncableobject.cpp | 17 ---------- src/common/syncableobject.h | 3 -- src/qtui/settingspages/aliasesmodel.cpp | 31 +++++++----------- src/qtui/settingspages/aliasesmodel.h | 13 +++----- .../corehighlightsettingspage.cpp | 4 +-- src/qtui/settingspages/dccsettingspage.cpp | 4 +-- src/qtui/settingspages/ignorelistmodel.cpp | 32 +++++++------------ src/qtui/settingspages/ignorelistmodel.h | 13 +++----- 14 files changed, 38 insertions(+), 114 deletions(-) diff --git a/src/common/aliasmanager.cpp b/src/common/aliasmanager.cpp index a439fb9f..f87cd585 100644 --- a/src/common/aliasmanager.cpp +++ b/src/common/aliasmanager.cpp @@ -25,16 +25,6 @@ #include "network.h" -AliasManager& AliasManager::operator=(const AliasManager& other) -{ - if (this == &other) - return *this; - - SyncableObject::operator=(other); - _aliases = other._aliases; - return *this; -} - int AliasManager::indexOf(const QString& name) const { for (int i = 0; i < _aliases.count(); i++) { diff --git a/src/common/aliasmanager.h b/src/common/aliasmanager.h index 937a5ab5..2ae16cc3 100644 --- a/src/common/aliasmanager.h +++ b/src/common/aliasmanager.h @@ -42,7 +42,6 @@ public: { setAllowClientUpdates(true); } - AliasManager& operator=(const AliasManager& other); struct Alias { diff --git a/src/common/highlightrulemanager.cpp b/src/common/highlightrulemanager.cpp index 37e2d869..5a62f9e6 100644 --- a/src/common/highlightrulemanager.cpp +++ b/src/common/highlightrulemanager.cpp @@ -25,18 +25,6 @@ #include "expressionmatch.h" #include "util.h" -HighlightRuleManager& HighlightRuleManager::operator=(const HighlightRuleManager& other) -{ - if (this == &other) - return *this; - - SyncableObject::operator=(other); - _highlightRuleList = other._highlightRuleList; - _nicksCaseSensitive = other._nicksCaseSensitive; - _highlightNick = other._highlightNick; - return *this; -} - int HighlightRuleManager::indexOf(int id) const { for (int i = 0; i < _highlightRuleList.count(); i++) { diff --git a/src/common/highlightrulemanager.h b/src/common/highlightrulemanager.h index 47be30a5..8f9a381d 100644 --- a/src/common/highlightrulemanager.h +++ b/src/common/highlightrulemanager.h @@ -55,7 +55,6 @@ public: { setAllowClientUpdates(true); } - HighlightRuleManager& operator=(const HighlightRuleManager& other); /** * Individual highlight rule diff --git a/src/common/ignorelistmanager.cpp b/src/common/ignorelistmanager.cpp index 83add359..d0ab71fb 100644 --- a/src/common/ignorelistmanager.cpp +++ b/src/common/ignorelistmanager.cpp @@ -23,16 +23,6 @@ #include #include -IgnoreListManager& IgnoreListManager::operator=(const IgnoreListManager& other) -{ - if (this == &other) - return *this; - - SyncableObject::operator=(other); - _ignoreList = other._ignoreList; - return *this; -} - int IgnoreListManager::indexOf(const QString& ignore) const { for (int i = 0; i < _ignoreList.count(); i++) { diff --git a/src/common/ignorelistmanager.h b/src/common/ignorelistmanager.h index c1dbf10b..ecb56fb3 100644 --- a/src/common/ignorelistmanager.h +++ b/src/common/ignorelistmanager.h @@ -43,7 +43,6 @@ public: { setAllowClientUpdates(true); } - IgnoreListManager& operator=(const IgnoreListManager& other); enum IgnoreType { diff --git a/src/common/syncableobject.cpp b/src/common/syncableobject.cpp index 22ebec28..825b97db 100644 --- a/src/common/syncableobject.cpp +++ b/src/common/syncableobject.cpp @@ -44,13 +44,6 @@ SyncableObject::SyncableObject(const QString& objectName, QObject* parent) }); } -SyncableObject::SyncableObject(const SyncableObject& other, QObject* parent) - : SyncableObject(QString{}, parent) -{ - _initialized = other._initialized; - _allowClientUpdates = other._allowClientUpdates; -} - SyncableObject::~SyncableObject() { QList::iterator proxyIter = _signalProxies.begin(); @@ -61,16 +54,6 @@ SyncableObject::~SyncableObject() } } -SyncableObject& SyncableObject::operator=(const SyncableObject& other) -{ - if (this == &other) - return *this; - - _initialized = other._initialized; - _allowClientUpdates = other._allowClientUpdates; - return *this; -} - bool SyncableObject::isInitialized() const { return _initialized; diff --git a/src/common/syncableobject.h b/src/common/syncableobject.h index 3e371c26..c2163026 100644 --- a/src/common/syncableobject.h +++ b/src/common/syncableobject.h @@ -58,7 +58,6 @@ class COMMON_EXPORT SyncableObject : public QObject public: SyncableObject(QObject* parent = nullptr); SyncableObject(const QString& objectName, QObject* parent = nullptr); - SyncableObject(const SyncableObject& other, QObject* parent = nullptr); ~SyncableObject() override; //! Stores the object's state into a QVariantMap. @@ -94,8 +93,6 @@ public slots: protected: void sync_call__(SignalProxy::ProxyMode modeType, const char* funcname, ...) const; - SyncableObject& operator=(const SyncableObject& other); - signals: void initDone(); void updatedRemotely(); diff --git a/src/qtui/settingspages/aliasesmodel.cpp b/src/qtui/settingspages/aliasesmodel.cpp index 42716145..780b152a 100644 --- a/src/qtui/settingspages/aliasesmodel.cpp +++ b/src/qtui/settingspages/aliasesmodel.cpp @@ -244,47 +244,41 @@ QModelIndex AliasesModel::index(int row, int column, const QModelIndex& parent) const AliasManager& AliasesModel::aliasManager() const { - if (_configChanged) - return _clonedAliasManager; - else - return *Client::aliasManager(); + return _clonedAliasManager ? *_clonedAliasManager : *Client::aliasManager(); } AliasManager& AliasesModel::aliasManager() { - if (_configChanged) - return _clonedAliasManager; - else - return *Client::aliasManager(); + return _clonedAliasManager ? *_clonedAliasManager : *Client::aliasManager(); } AliasManager& AliasesModel::cloneAliasManager() { - if (!_configChanged) { - _clonedAliasManager = *Client::aliasManager(); - _configChanged = true; + if (!_clonedAliasManager) { + _clonedAliasManager = std::make_unique(); + _clonedAliasManager->fromVariantMap(Client::aliasManager()->toVariantMap()); emit configChanged(true); } - return _clonedAliasManager; + return *_clonedAliasManager; } void AliasesModel::revert() { - if (!_configChanged) + if (!_clonedAliasManager) return; - _configChanged = false; - emit configChanged(false); beginResetModel(); + _clonedAliasManager.reset(); endResetModel(); + emit configChanged(false); } void AliasesModel::commit() { - if (!_configChanged) + if (!_clonedAliasManager) return; - Client::aliasManager()->requestUpdate(_clonedAliasManager.toVariantMap()); + Client::aliasManager()->requestUpdate(_clonedAliasManager->toVariantMap()); revert(); } @@ -307,10 +301,9 @@ void AliasesModel::clientConnected() void AliasesModel::clientDisconnected() { - // clear - _clonedAliasManager = ClientAliasManager(); _modelReady = false; beginResetModel(); + _clonedAliasManager.reset(); endResetModel(); emit modelReady(false); } diff --git a/src/qtui/settingspages/aliasesmodel.h b/src/qtui/settingspages/aliasesmodel.h index ba0674b7..3dc7f0d4 100644 --- a/src/qtui/settingspages/aliasesmodel.h +++ b/src/qtui/settingspages/aliasesmodel.h @@ -18,11 +18,11 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#ifndef ALIASESMODEL_H -#define ALIASESMODEL_H +#pragma once + +#include #include -#include #include "clientaliasmanager.h" @@ -47,7 +47,7 @@ public: inline int rowCount(const QModelIndex& parent = QModelIndex()) const override; inline int columnCount(const QModelIndex& parent = QModelIndex()) const override; - inline bool hasConfigChanged() const { return _configChanged; } + inline bool hasConfigChanged() const { return static_cast(_clonedAliasManager); } inline bool isReady() const { return _modelReady; } public slots: @@ -62,8 +62,7 @@ signals: void modelReady(bool); private: - ClientAliasManager _clonedAliasManager; - bool _configChanged{false}; + std::unique_ptr _clonedAliasManager; bool _modelReady{false}; const AliasManager& aliasManager() const; @@ -88,5 +87,3 @@ int AliasesModel::columnCount(const QModelIndex& parent) const Q_UNUSED(parent); return isReady() ? 2 : 0; } - -#endif // ALIASESMODEL_H diff --git a/src/qtui/settingspages/corehighlightsettingspage.cpp b/src/qtui/settingspages/corehighlightsettingspage.cpp index 9e968ee7..5a53b4ab 100644 --- a/src/qtui/settingspages/corehighlightsettingspage.cpp +++ b/src/qtui/settingspages/corehighlightsettingspage.cpp @@ -620,7 +620,7 @@ void CoreHighlightSettingsPage::save() if (ruleManager == nullptr) return; - auto clonedManager = HighlightRuleManager(); + HighlightRuleManager clonedManager; clonedManager.fromVariantMap(ruleManager->toVariantMap()); clonedManager.clear(); @@ -734,7 +734,7 @@ void CoreHighlightSettingsPage::importRules() save(); } - auto clonedManager = HighlightRuleManager(); + HighlightRuleManager clonedManager; clonedManager.fromVariantMap(Client::highlightRuleManager()->toVariantMap()); for (const auto& variant : notificationSettings.highlightList()) { diff --git a/src/qtui/settingspages/dccsettingspage.cpp b/src/qtui/settingspages/dccsettingspage.cpp index 56d22caf..3d744497 100644 --- a/src/qtui/settingspages/dccsettingspage.cpp +++ b/src/qtui/settingspages/dccsettingspage.cpp @@ -81,14 +81,14 @@ bool DccSettingsPage::hasDefaults() const void DccSettingsPage::defaults() { - _localConfig = DccConfig(); + _localConfig.fromVariantMap(DccConfig{}.toVariantMap()); SettingsPage::load(); widgetHasChanged(); } void DccSettingsPage::load() { - _localConfig = isClientConfigValid() ? *_clientConfig : DccConfig{}; + _localConfig.fromVariantMap(isClientConfigValid() ? _clientConfig->toVariantMap() : DccConfig{}.toVariantMap()); SettingsPage::load(); widgetHasChanged(); } diff --git a/src/qtui/settingspages/ignorelistmodel.cpp b/src/qtui/settingspages/ignorelistmodel.cpp index 3d14ddc9..d08a280d 100644 --- a/src/qtui/settingspages/ignorelistmodel.cpp +++ b/src/qtui/settingspages/ignorelistmodel.cpp @@ -144,7 +144,6 @@ bool IgnoreListModel::newIgnoreRule(const IgnoreListManager::IgnoreListItem& ite if (manager.contains(item.contents())) return false; beginInsertRows(QModelIndex(), rowCount(), rowCount()); - // manager.addIgnoreListItem(item); manager.addIgnoreListItem(item.type(), item.contents(), item.isRegEx(), item.strictness(), item.scope(), item.scopeRule(), item.isEnabled()); endInsertRows(); return true; @@ -216,47 +215,41 @@ QModelIndex IgnoreListModel::index(int row, int column, const QModelIndex& paren const IgnoreListManager& IgnoreListModel::ignoreListManager() const { - if (_configChanged) - return _clonedIgnoreListManager; - else - return *Client::ignoreListManager(); + return _clonedIgnoreListManager ? *_clonedIgnoreListManager : *Client::ignoreListManager(); } IgnoreListManager& IgnoreListModel::ignoreListManager() { - if (_configChanged) - return _clonedIgnoreListManager; - else - return *Client::ignoreListManager(); + return _clonedIgnoreListManager ? *_clonedIgnoreListManager : *Client::ignoreListManager(); } IgnoreListManager& IgnoreListModel::cloneIgnoreListManager() { - if (!_configChanged) { - _clonedIgnoreListManager = *Client::ignoreListManager(); - _configChanged = true; + if (!_clonedIgnoreListManager) { + _clonedIgnoreListManager = std::make_unique(); + _clonedIgnoreListManager->fromVariantMap(Client::ignoreListManager()->toVariantMap()); emit configChanged(true); } - return _clonedIgnoreListManager; + return *_clonedIgnoreListManager; } void IgnoreListModel::revert() { - if (!_configChanged) + if (!_clonedIgnoreListManager) return; - _configChanged = false; - emit configChanged(false); beginResetModel(); + _clonedIgnoreListManager.reset(); endResetModel(); + emit configChanged(false); } void IgnoreListModel::commit() { - if (!_configChanged) + if (!_clonedIgnoreListManager) return; - Client::ignoreListManager()->requestUpdate(_clonedIgnoreListManager.toVariantMap()); + Client::ignoreListManager()->requestUpdate(_clonedIgnoreListManager->toVariantMap()); revert(); } @@ -279,10 +272,9 @@ void IgnoreListModel::clientConnected() void IgnoreListModel::clientDisconnected() { - // clear - _clonedIgnoreListManager = ClientIgnoreListManager(); _modelReady = false; beginResetModel(); + _clonedIgnoreListManager.reset(); endResetModel(); emit modelReady(false); } diff --git a/src/qtui/settingspages/ignorelistmodel.h b/src/qtui/settingspages/ignorelistmodel.h index 3b27a1c7..262358b5 100644 --- a/src/qtui/settingspages/ignorelistmodel.h +++ b/src/qtui/settingspages/ignorelistmodel.h @@ -18,11 +18,11 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#ifndef IGNORELISTMODEL_H -#define IGNORELISTMODEL_H +#pragma once + +#include #include -#include #include "clientignorelistmanager.h" @@ -47,7 +47,7 @@ public: inline int rowCount(const QModelIndex& parent = QModelIndex()) const override; inline int columnCount(const QModelIndex& parent = QModelIndex()) const override; - inline bool hasConfigChanged() const { return _configChanged; } + inline bool hasConfigChanged() const { return static_cast(_clonedIgnoreListManager); } inline bool isReady() const { return _modelReady; } const IgnoreListManager::IgnoreListItem& ignoreListItemAt(int row) const; @@ -66,8 +66,7 @@ signals: void modelReady(bool); private: - ClientIgnoreListManager _clonedIgnoreListManager; - bool _configChanged{false}; + std::unique_ptr _clonedIgnoreListManager; bool _modelReady{false}; const IgnoreListManager& ignoreListManager() const; @@ -92,5 +91,3 @@ int IgnoreListModel::columnCount(const QModelIndex& parent) const Q_UNUSED(parent); return isReady() ? 3 : 0; } - -#endif // IGNORELISTMODEL_H -- 2.20.1