From f1db4d6e385deae976b6bf9b3cd392b794e09383 Mon Sep 17 00:00:00 2001
From: Lars Knoll <lars.knoll@qt.io>
Date: Tue, 10 Nov 2020 11:30:04 +0100
Subject: [PATCH] Fix moveAppend() implementation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When appending multiple items, we are fine with providing
weak exception safety only. This implies that we can simplify
the moveAppend() code and avoid having to potentiall
call destructors in there.

Change-Id: I31cef0e8589e28f3d3521c54db3f7910628e686f
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/tools/qarraydataops.h             | 22 +++-----
 .../tools/qarraydata/tst_qarraydata.cpp       | 53 +------------------
 2 files changed, 7 insertions(+), 68 deletions(-)

diff --git a/src/corelib/tools/qarraydataops.h b/src/corelib/tools/qarraydataops.h
index e0f3484bcb4..9e5057099e6 100644
--- a/src/corelib/tools/qarraydataops.h
+++ b/src/corelib/tools/qarraydataops.h
@@ -113,15 +113,6 @@ struct QArrayExceptionSafetyPrimitives
             }
             return qsizetype(std::exchange(n, 0));
         }
-        qsizetype move(T *first, T *last) noexcept(std::is_nothrow_move_constructible_v<T>)
-        {
-            n = 0;
-            for (; first != last; ++first) {
-                new (where + n) T(std::move(*first));
-                ++n;
-            }
-            return qsizetype(std::exchange(n, 0));
-        }
         ~Constructor() noexcept(std::is_nothrow_destructible_v<T>)
         {
             while (n)
@@ -544,13 +535,12 @@ public:
         if (b == e)
             return;
 
-        typedef typename QArrayExceptionSafetyPrimitives<T>::Constructor CopyConstructor;
-
-        // Provides strong exception safety guarantee,
-        // provided T::~T() nothrow
-
-        CopyConstructor copier(this->end());
-        this->size += copier.move(b, e);
+        T *data = this->begin();
+        while (b < e) {
+            new (data + this->size) T(std::move(*b));
+            ++b;
+            ++this->size;
+        }
     }
 
     void truncate(size_t newSize)
diff --git a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
index 9aa1b4e6684..f39389fa60e 100644
--- a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
+++ b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
@@ -2375,7 +2375,7 @@ void tst_QArrayData::exceptionSafetyPrimitives_constructor()
     {
         auto data = createDataPointer<ThrowingType>(20, 10);
         const auto originalSize = data.size;
-        std::array<ThrowingType, 0> emptyRange{};
+        const std::array<ThrowingType, 0> emptyRange{};
 
         doConstruction(data, data.end(), [] (Constructor &ctor) { return ctor.create(0); });
         QCOMPARE(data.size, originalSize);
@@ -2389,11 +2389,6 @@ void tst_QArrayData::exceptionSafetyPrimitives_constructor()
             return ctor.clone(0, ThrowingType(42));
         });
         QCOMPARE(data.size, originalSize);
-
-        doConstruction(data, data.end(), [emptyRange] (Constructor &ctor) mutable {
-            return ctor.move(emptyRange.begin(), emptyRange.end());
-        });
-        QCOMPARE(data.size, originalSize);
     }
 
     // successful create
@@ -2437,24 +2432,6 @@ void tst_QArrayData::exceptionSafetyPrimitives_constructor()
             QCOMPARE(data.data()[i], reference.data()[i]);
     }
 
-    // successful move
-    {
-        auto data = createDataPointer<ThrowingType>(20, 10);
-        auto reference = createDataPointer<ThrowingType>(20, 10);
-        std::array<ThrowingType, 3> source = {
-            ThrowingType(42), ThrowingType(43), ThrowingType(44)
-        };
-        reference->copyAppend(source.begin(), source.end());
-
-        doConstruction(data, data.end(), [source] (Constructor &ctor) mutable {
-            return ctor.move(source.begin(), source.end());
-        });
-
-        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<ThrowingType>(20, 10);
@@ -2528,34 +2505,6 @@ void tst_QArrayData::exceptionSafetyPrimitives_constructor()
             }
         }
     }
-
-    // failed move
-    {
-        auto data = createDataPointer<ThrowingType>(20, 10);
-        auto reference = createDataPointer<ThrowingType>(20, 10);
-        std::array<ThrowingType, 4> source = {
-            ThrowingType(42), ThrowingType(43), ThrowingType(44), ThrowingType(170)
-        };
-
-        for (uint throwOnNthConstruction : {1, 3}) {
-            WatcherScope scope; Q_UNUSED(scope);
-            try {
-                ThrowingType::throwOnce = throwOnNthConstruction;
-                doConstruction(data, data.end(), [source] (Constructor &ctor) mutable {
-                    return ctor.move(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]);
-            }
-        }
-    }
 }
 
 void tst_QArrayData::exceptionSafetyPrimitives_destructor()