From c33ee0fe1517021ff2f22a8aab0331e4145c5f3f Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Mon, 20 Mar 2017 12:34:29 +0100 Subject: [PATCH] QDataWidgetMapper: replace an inefficient QList with std::vector WidgetMapper is larger than a void*, so holding it in QLists is needlessly inefficient. Worse, the code could come to depend on the fragile property of (inefficient) QLists that references to elements therein never are invalidated. Fix by holding it in a std::vector instead (tried QVector, too, but that produced almost exactly 1KiB more text size). Adapt to std API. There are a few changes in there which are not strictly necessary when using std::vector, but were done as part of the port to QVector: - dropping of WidgetMapper ctors (because QVector requires a default ctor, and I didn't want to provide one manually), - marking the type as movable (no effect with std::vector, but doesn't hurt and keeps the next guy from wondering), and - marking Private::flipEventFilters() const (to avoid a detach when using ranged for loops). Changes required by the port to std::vector: - at() -> op[] (not strictly necessary, but expands to less code even when compiling, as QtWidgets is, without exception support), - append() -> push_back() Saves 40B in text size on optimized GCC 6.1 Linux AMD64 builds (for comparison: QVector had 1240B more than QList). Change-Id: Iaae81d1a8bebe8000bf69e7fb8b2bcd6c38afafd Reviewed-by: Edward Welbourne Reviewed-by: Lars Knoll --- src/widgets/itemviews/qdatawidgetmapper.cpp | 58 ++++++++++----------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/widgets/itemviews/qdatawidgetmapper.cpp b/src/widgets/itemviews/qdatawidgetmapper.cpp index b213a0859c7..99704c79114 100644 --- a/src/widgets/itemviews/qdatawidgetmapper.cpp +++ b/src/widgets/itemviews/qdatawidgetmapper.cpp @@ -89,11 +89,11 @@ public: : model->index(itemPos, currentIdx(), rootIndex); } - inline void flipEventFilters(QAbstractItemDelegate *oldDelegate, - QAbstractItemDelegate *newDelegate) + void flipEventFilters(QAbstractItemDelegate *oldDelegate, + QAbstractItemDelegate *newDelegate) const { - for (QList::const_iterator it = widgetMap.cbegin(), end = widgetMap.cend(); it != end; ++it) { - QWidget *w = it->widget; + for (const WidgetMapper &e : widgetMap) { + QWidget *w = e.widget; if (!w) continue; w->removeEventFilter(oldDelegate); @@ -111,11 +111,6 @@ public: struct WidgetMapper { - inline WidgetMapper(QWidget *w = 0, int c = 0, const QModelIndex &i = QModelIndex()) - : widget(w), section(c), currentIndex(i) {} - inline WidgetMapper(QWidget *w, int c, const QModelIndex &i, const QByteArray &p) - : widget(w), section(c), currentIndex(i), property(p) {} - QPointer widget; int section; QPersistentModelIndex currentIndex; @@ -127,14 +122,15 @@ public: bool commit(const WidgetMapper &m); - QList widgetMap; + std::vector widgetMap; }; +Q_DECLARE_TYPEINFO(QDataWidgetMapperPrivate::WidgetMapper, Q_MOVABLE_TYPE); int QDataWidgetMapperPrivate::findWidget(QWidget *w) const { - for (QList::const_iterator it = widgetMap.cbegin(), end = widgetMap.cend(); it != end; ++it) { - if (it->widget == w) - return int(std::distance(widgetMap.cbegin(), it)); + for (const WidgetMapper &e : widgetMap) { + if (e.widget == w) + return int(&e - &widgetMap.front()); } return -1; } @@ -171,8 +167,8 @@ void QDataWidgetMapperPrivate::populate(WidgetMapper &m) void QDataWidgetMapperPrivate::populate() { - for (QList::iterator it = widgetMap.begin(), end = widgetMap.end(); it != end; ++it) - populate(*it); + for (WidgetMapper &e : widgetMap) + populate(e); } static bool qContainsIndex(const QModelIndex &idx, const QModelIndex &topLeft, @@ -187,9 +183,9 @@ void QDataWidgetMapperPrivate::_q_dataChanged(const QModelIndex &topLeft, const if (topLeft.parent() != rootIndex) return; // not in our hierarchy - for (QList::iterator it = widgetMap.begin(), end = widgetMap.end(); it != end; ++it) { - if (qContainsIndex(it->currentIndex, topLeft, bottomRight)) - populate(*it); + for (WidgetMapper &e : widgetMap) { + if (qContainsIndex(e.currentIndex, topLeft, bottomRight)) + populate(e); } } @@ -202,7 +198,7 @@ void QDataWidgetMapperPrivate::_q_commitData(QWidget *w) if (idx == -1) return; // not our widget - commit(widgetMap.at(idx)); + commit(widgetMap[idx]); } void QDataWidgetMapperPrivate::_q_closeEditor(QWidget *w, QAbstractItemDelegate::EndEditHint hint) @@ -479,7 +475,7 @@ void QDataWidgetMapper::addMapping(QWidget *widget, int section) Q_D(QDataWidgetMapper); removeMapping(widget); - d->widgetMap.append(QDataWidgetMapperPrivate::WidgetMapper(widget, section, d->indexAt(section))); + d->widgetMap.push_back({widget, section, d->indexAt(section), QByteArray()}); widget->installEventFilter(d->delegate); } @@ -497,7 +493,7 @@ void QDataWidgetMapper::addMapping(QWidget *widget, int section, const QByteArra Q_D(QDataWidgetMapper); removeMapping(widget); - d->widgetMap.append(QDataWidgetMapperPrivate::WidgetMapper(widget, section, d->indexAt(section), propertyName)); + d->widgetMap.push_back({widget, section, d->indexAt(section), propertyName}); widget->installEventFilter(d->delegate); } @@ -514,7 +510,7 @@ void QDataWidgetMapper::removeMapping(QWidget *widget) if (idx == -1) return; - d->widgetMap.removeAt(idx); + d->widgetMap.erase(d->widgetMap.begin() + idx); widget->removeEventFilter(d->delegate); } @@ -532,7 +528,7 @@ int QDataWidgetMapper::mappedSection(QWidget *widget) const if (idx == -1) return -1; - return d->widgetMap.at(idx).section; + return d->widgetMap[idx].section; } /*! @@ -550,7 +546,7 @@ QByteArray QDataWidgetMapper::mappedPropertyName(QWidget *widget) const int idx = d->findWidget(widget); if (idx == -1) return QByteArray(); - const QDataWidgetMapperPrivate::WidgetMapper &m = d->widgetMap.at(idx); + const auto &m = d->widgetMap[idx]; if (m.property.isEmpty()) return m.widget->metaObject()->userProperty().name(); else @@ -567,9 +563,9 @@ QWidget *QDataWidgetMapper::mappedWidgetAt(int section) const { Q_D(const QDataWidgetMapper); - for (QList::const_iterator it = d->widgetMap.cbegin(), end = d->widgetMap.cend(); it != end; ++it) { - if (it->section == section) - return it->widget; + for (auto &e : d->widgetMap) { + if (e.section == section) + return e.widget; } return 0; @@ -606,8 +602,8 @@ bool QDataWidgetMapper::submit() { Q_D(QDataWidgetMapper); - for (QList::const_iterator it = d->widgetMap.cbegin(), end = d->widgetMap.cend(); it != end; ++it) { - if (!d->commit(*it)) + for (auto &e : d->widgetMap) { + if (!d->commit(e)) return false; } @@ -746,9 +742,9 @@ void QDataWidgetMapper::clearMapping() { Q_D(QDataWidgetMapper); - QList copy; + decltype(d->widgetMap) copy; d->widgetMap.swap(copy); // a C++98 move - for (QList::const_reverse_iterator it = copy.crbegin(), end = copy.crend(); it != end; ++it) { + for (auto it = copy.crbegin(), end = copy.crend(); it != end; ++it) { if (it->widget) it->widget->removeEventFilter(d->delegate); }