From 6431565e0a62365259196b02c1aec892d9f85a0a Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 11 Nov 2020 14:51:32 +0100 Subject: [PATCH] Simplify QArrayDataOps::insert() for movable types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid ever having to call a destructor and unify the code for insertion at the front or at the end. Change-Id: Ie50ae2d4a75477cfdae9d5bd4bddf268426d95b5 Reviewed-by: Andrei Golubev Reviewed-by: Mårten Nordheim --- src/corelib/tools/qarraydataops.h | 225 ++++++------------ .../tools/qarraydata/tst_qarraydata.cpp | 157 ------------ 2 files changed, 79 insertions(+), 303 deletions(-) diff --git a/src/corelib/tools/qarraydataops.h b/src/corelib/tools/qarraydataops.h index fe81e66a826..028d63efc2c 100644 --- a/src/corelib/tools/qarraydataops.h +++ b/src/corelib/tools/qarraydataops.h @@ -76,50 +76,6 @@ struct QArrayExceptionSafetyPrimitives using parameter_type = typename QArrayDataPointer::parameter_type; using iterator = typename QArrayDataPointer::iterator; - // Constructs a range of elements at the specified position. If an exception - // is thrown during construction, already constructed elements are - // destroyed. By design, only one function (create/copy/clone/move) and only - // once is supposed to be called per class instance. - struct Constructor - { - T *const where; - size_t n = 0; - - Constructor(T *w) noexcept : where(w) {} - qsizetype create(size_t size) noexcept(std::is_nothrow_default_constructible_v) - { - n = 0; - while (n != size) { - new (where + n) T; - ++n; - } - return qsizetype(std::exchange(n, 0)); - } - qsizetype copy(const T *first, const T *last) noexcept(std::is_nothrow_copy_constructible_v) - { - n = 0; - for (; first != last; ++first) { - new (where + n) T(*first); - ++n; - } - return qsizetype(std::exchange(n, 0)); - } - qsizetype clone(size_t size, parameter_type t) noexcept(std::is_nothrow_constructible_v) - { - n = 0; - while (n != size) { - new (where + n) T(t); - ++n; - } - return qsizetype(std::exchange(n, 0)); - } - ~Constructor() noexcept(std::is_nothrow_destructible_v) - { - while (n) - where[--n].~T(); - } - }; - // Moves the data range in memory by the specified amount. Unless commit() // is called, the data is moved back to the original place at the end of // object lifetime. @@ -141,9 +97,10 @@ struct QArrayExceptionSafetyPrimitives void commit() noexcept { displace = 0; } ~Displacer() noexcept { - if (displace) - ::memmove(static_cast(begin), static_cast(begin + displace), - (end - begin) * sizeof(T)); + if constexpr (!std::is_nothrow_copy_constructible_v) + if (displace) + ::memmove(static_cast(begin), static_cast(begin + displace), + (end - begin) * sizeof(T)); } }; }; @@ -829,6 +786,79 @@ public: // using QGenericArrayOps::destroyAll; typedef typename QGenericArrayOps::parameter_type parameter_type; + struct Inserter + { + QArrayDataPointer *data; + T *displaceFrom; + T *displaceTo; + qsizetype nInserts = 0; + qsizetype bytes; + + qsizetype increment = 1; + + Inserter(QArrayDataPointer *d, QArrayData::GrowthPosition pos) + : data(d), increment(pos == QArrayData::GrowsAtBeginning ? -1 : 1) + { + } + ~Inserter() { + if constexpr (!std::is_nothrow_copy_constructible_v) { + if (displaceFrom != displaceTo) { + ::memmove(static_cast(displaceFrom), static_cast(displaceTo), bytes); + nInserts -= qAbs(displaceFrom - displaceTo); + } + } + if (increment < 0) + data->ptr -= nInserts; + data->size += nInserts; + } + + T *displace(qsizetype pos, qsizetype n) + { + nInserts = n; + T *insertionPoint = data->ptr + pos; + if (increment > 0) { + displaceFrom = data->ptr + pos; + displaceTo = displaceFrom + n; + bytes = data->size - pos; + } else { + displaceFrom = data->ptr; + displaceTo = displaceFrom - n; + --insertionPoint; + bytes = pos; + } + bytes *= sizeof(T); + ::memmove(static_cast(displaceTo), static_cast(displaceFrom), bytes); + return insertionPoint; + } + + void insert(qsizetype pos, const T *source, qsizetype n) + { + T *where = displace(pos, n); + + if (increment < 0) + source += n - 1; + + while (n--) { + new (where) T(*source); + where += increment; + source += increment; + displaceFrom += increment; + } + } + + void insert(qsizetype pos, const T &t, qsizetype n) + { + T *where = displace(pos, n); + + while (n--) { + new (where) T(t); + where += increment; + displaceFrom += increment; + } + } + }; + + void insert(qsizetype i, const T *data, qsizetype n) { typename Data::GrowthPosition pos = Data::GrowsAtEnd; @@ -839,56 +869,7 @@ public: Q_ASSERT((pos == Data::GrowsAtBeginning && this->freeSpaceAtBegin() >= n) || (pos == Data::GrowsAtEnd && this->freeSpaceAtEnd() >= n)); - T *where = this->begin() + i; - if (pos == QArrayData::GrowsAtBeginning) - insert(GrowsBackwardsTag{}, where, data, data + n); - else - insert(GrowsForwardTag{}, where, data, data + n); - } - - void insert(GrowsForwardTag, T *where, const T *b, const T *e) - { - Q_ASSERT(this->isMutable() || (b == e && where == this->end())); - Q_ASSERT(!this->isShared() || (b == e && where == this->end())); - Q_ASSERT(where >= this->begin() && where <= this->end()); - Q_ASSERT(b < e); - Q_ASSERT(e <= where || b > this->end() || where == this->end()); // No overlap or append - Q_ASSERT((e - b) <= this->freeSpaceAtEnd()); - - typedef typename QArrayExceptionSafetyPrimitives::Displacer ReversibleDisplace; - typedef typename QArrayExceptionSafetyPrimitives::Constructor CopyConstructor; - - // Provides strong exception safety guarantee, - // provided T::~T() nothrow - - ReversibleDisplace displace(where, this->end(), e - b); - CopyConstructor copier(where); - const auto copiedSize = copier.copy(b, e); - displace.commit(); - this->size += copiedSize; - } - - void insert(GrowsBackwardsTag, T *where, const T *b, const T *e) - { - Q_ASSERT(this->isMutable() || (b == e && where == this->end())); - Q_ASSERT(!this->isShared() || (b == e && where == this->end())); - Q_ASSERT(where >= this->begin() && where <= this->end()); - Q_ASSERT(b < e); - Q_ASSERT(e <= where || b > this->end() || where == this->end()); // No overlap or append - Q_ASSERT((e - b) <= this->freeSpaceAtBegin()); - - typedef typename QArrayExceptionSafetyPrimitives::Constructor CopyConstructor; - typedef typename QArrayExceptionSafetyPrimitives::Displacer ReversibleDisplace; - - // Provides strong exception safety guarantee, - // provided T::~T() nothrow - - ReversibleDisplace displace(this->begin(), where, -(e - b)); - CopyConstructor copier(where - (e - b)); - const auto copiedSize = copier.copy(b, e); - displace.commit(); - this->ptr -= copiedSize; - this->size += copiedSize; + Inserter(this, pos).insert(i, data, n); } void insert(qsizetype i, qsizetype n, parameter_type t) @@ -902,57 +883,9 @@ public: Q_ASSERT((pos == Data::GrowsAtBeginning && this->freeSpaceAtBegin() >= n) || (pos == Data::GrowsAtEnd && this->freeSpaceAtEnd() >= n)); - T *where = this->begin() + i; - if (pos == QArrayData::GrowsAtBeginning) - insert(GrowsBackwardsTag{}, where, n, copy); - else - insert(GrowsForwardTag{}, where, n, copy); + Inserter(this, pos).insert(i, copy, n); } - void insert(GrowsForwardTag, T *where, size_t n, parameter_type t) - { - Q_ASSERT(!this->isShared()); - Q_ASSERT(n); - Q_ASSERT(where >= this->begin() && where <= this->end()); - Q_ASSERT(size_t(this->freeSpaceAtEnd()) >= n); - - typedef typename QArrayExceptionSafetyPrimitives::Displacer ReversibleDisplace; - typedef typename QArrayExceptionSafetyPrimitives::Constructor CopyConstructor; - - // Provides strong exception safety guarantee, - // provided T::~T() nothrow - - ReversibleDisplace displace(where, this->end(), qsizetype(n)); - CopyConstructor copier(where); - const auto copiedSize = copier.clone(n, t); - displace.commit(); - this->size += copiedSize; - } - - void insert(GrowsBackwardsTag, T *where, size_t n, parameter_type t) - { - Q_ASSERT(!this->isShared()); - Q_ASSERT(n); - Q_ASSERT(where >= this->begin() && where <= this->end()); - Q_ASSERT(size_t(this->freeSpaceAtBegin()) >= n); - - typedef typename QArrayExceptionSafetyPrimitives::Constructor CopyConstructor; - typedef typename QArrayExceptionSafetyPrimitives::Displacer ReversibleDisplace; - - // Provides strong exception safety guarantee, - // provided T::~T() nothrow - - ReversibleDisplace displace(this->begin(), where, -qsizetype(n)); - CopyConstructor copier(where - n); - const auto copiedSize = copier.clone(n, t); - displace.commit(); - this->ptr -= copiedSize; - this->size += copiedSize; - } - - // use moving insert - using QGenericArrayOps::insert; - template void emplace(GrowsForwardTag, T *where, Args &&... args) { diff --git a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp index a5d2967c4a5..7ab93d636ad 100644 --- a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp +++ b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp @@ -85,7 +85,6 @@ private slots: void selfEmplaceBackwards(); void selfEmplaceForward(); #ifndef QT_NO_EXCEPTIONS - void exceptionSafetyPrimitives_constructor(); void exceptionSafetyPrimitives_displacer(); #endif }; @@ -2349,162 +2348,6 @@ static QArrayDataPointer createDataPointer(qsizetype capacity, qsizetype init return adp; } -void tst_QArrayData::exceptionSafetyPrimitives_constructor() -{ - using Prims = QtPrivate::QArrayExceptionSafetyPrimitives; - using Constructor = typename Prims::Constructor; - - struct WatcherScope - { - WatcherScope() { throwingTypeWatcher().watch = true; } - ~WatcherScope() - { - throwingTypeWatcher().watch = false; - throwingTypeWatcher().destroyedIds.clear(); - } - }; - - const auto doConstruction = [] (auto &dataPointer, auto where, auto op) { - Constructor ctor(where); - dataPointer.size += op(ctor); - }; - - // empty ranges - { - auto data = createDataPointer(20, 10); - const auto originalSize = data.size; - const std::array emptyRange{}; - - doConstruction(data, data.end(), [] (Constructor &ctor) { return ctor.create(0); }); - QCOMPARE(data.size, originalSize); - - doConstruction(data, data.end(), [&emptyRange] (Constructor &ctor) { - return ctor.copy(emptyRange.begin(), emptyRange.end()); - }); - QCOMPARE(data.size, originalSize); - - doConstruction(data, data.end(), [] (Constructor &ctor) { - return ctor.clone(0, ThrowingType(42)); - }); - QCOMPARE(data.size, originalSize); - } - - // successful create - { - auto data = createDataPointer(20, 10); - auto reference = createDataPointer(20, 10); - reference->appendInitialize(reference.size + 1); - - doConstruction(data, data.end(), [] (Constructor &ctor) { return ctor.create(1); }); - - QCOMPARE(data.size, reference.size); - for (qsizetype i = 0; i < data.size; ++i) - QCOMPARE(data.data()[i], reference.data()[i]); - } - - // successful copy - { - auto data = createDataPointer(20, 10); - auto reference = createDataPointer(20, 10); - const std::array source = { - ThrowingType(42), ThrowingType(43), ThrowingType(44) - }; - reference->copyAppend(source.begin(), source.end()); - - doConstruction(data, data.end(), [&source] (Constructor &ctor) { - return ctor.copy(source.begin(), source.end()); - }); - - QCOMPARE(data.size, reference.size); - for (qsizetype i = 0; i < data.size; ++i) - QCOMPARE(data.data()[i], reference.data()[i]); - - reference->copyAppend(2, source[0]); - - doConstruction(data, data.end(), [&source] (Constructor &ctor) { - return ctor.clone(2, source[0]); - }); - - QCOMPARE(data.size, reference.size); - for (qsizetype i = 0; i < data.size; ++i) - QCOMPARE(data.data()[i], reference.data()[i]); - } - - // failed create - { - auto data = createDataPointer(20, 10); - auto reference = createDataPointer(20, 10); - - for (uint throwOnNthConstruction : {1, 3}) { - WatcherScope scope; Q_UNUSED(scope); - try { - ThrowingType::throwOnce = throwOnNthConstruction; - doConstruction(data, data.end(), [] (Constructor &ctor) { - return ctor.create(5); - }); - } catch (const std::runtime_error &e) { - QCOMPARE(std::string(e.what()), ThrowingType::throwString); - QCOMPARE(data.size, reference.size); - for (qsizetype i = 0; i < data.size; ++i) - QCOMPARE(data.data()[i], reference.data()[i]); - QCOMPARE(throwingTypeWatcher().destroyedIds.size(), (throwOnNthConstruction - 1)); - for (auto id : throwingTypeWatcher().destroyedIds) - QCOMPARE(id, 0); - } - } - } - - // failed copy - { - auto data = createDataPointer(20, 10); - auto reference = createDataPointer(20, 10); - const std::array source = { - ThrowingType(42), ThrowingType(43), ThrowingType(44), ThrowingType(170) - }; - - // copy range - for (uint throwOnNthConstruction : {1, 3}) { - WatcherScope scope; Q_UNUSED(scope); - try { - ThrowingType::throwOnce = throwOnNthConstruction; - doConstruction(data, data.end(), [&source] (Constructor &ctor) { - return ctor.copy(source.begin(), source.end()); - }); - } catch (const std::runtime_error &e) { - QCOMPARE(std::string(e.what()), ThrowingType::throwString); - QCOMPARE(data.size, reference.size); - for (qsizetype i = 0; i < data.size; ++i) - QCOMPARE(data.data()[i], reference.data()[i]); - const auto destroyedSize = throwingTypeWatcher().destroyedIds.size(); - QCOMPARE(destroyedSize, (throwOnNthConstruction - 1)); - for (size_t i = 0; i < destroyedSize; ++i) - QCOMPARE(throwingTypeWatcher().destroyedIds[i], source[destroyedSize - i - 1]); - } - } - - // copy value - for (uint throwOnNthConstruction : {1, 3}) { - const ThrowingType value(512); - QVERIFY(QArrayDataPointer::pass_parameter_by_value == false); - WatcherScope scope; Q_UNUSED(scope); - try { - ThrowingType::throwOnce = throwOnNthConstruction; - doConstruction(data, data.end(), [&value] (Constructor &ctor) { - return ctor.clone(5, value); - }); - } catch (const std::runtime_error &e) { - QCOMPARE(std::string(e.what()), ThrowingType::throwString); - QCOMPARE(data.size, reference.size); - for (qsizetype i = 0; i < data.size; ++i) - QCOMPARE(data.data()[i], reference.data()[i]); - QCOMPARE(throwingTypeWatcher().destroyedIds.size(), (throwOnNthConstruction - 1)); - for (auto id : throwingTypeWatcher().destroyedIds) - QCOMPARE(id, 512); - } - } - } -} - void tst_QArrayData::exceptionSafetyPrimitives_displacer() { QVERIFY(QTypeInfo::isRelocatable);