From 5247af96e3bdf2f28f6e18007fb7e3a7458e0103 Mon Sep 17 00:00:00 2001
From: Andrei Golubev <andrei.golubev@qt.io>
Date: Wed, 8 Jul 2020 12:52:59 +0300
Subject: [PATCH] Refine precoditions and logic of array operations

Updated insert() methods:
* Refined Q_ASSERT() checks
* Fixed implementation issues (some of which resulted in
  actual crashes)
* Allowed to insert at the end. This is safe as far as I can
  tell and actually would allow to simplify considerable chunks
  of code (mainly, copyAppend versions to just return
  insert at the end)

Updated tests accordingly

Change-Id: I0ba33ae5034ce8d5ff95b753894e95d71ba00257
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/tools/qarraydataops.h                | 16 ++++++++--------
 .../corelib/tools/qarraydata/tst_qarraydata.cpp  | 13 +++++++++----
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/corelib/tools/qarraydataops.h b/src/corelib/tools/qarraydataops.h
index a97f2da175c..c3faa120c4f 100644
--- a/src/corelib/tools/qarraydataops.h
+++ b/src/corelib/tools/qarraydataops.h
@@ -145,7 +145,7 @@ public:
     {
         Q_ASSERT(this->isMutable());
         Q_ASSERT(!this->isShared());
-        Q_ASSERT(where >= this->begin() && where < this->end()); // Use copyAppend at end
+        Q_ASSERT(where >= this->begin() && where <= this->end());
         Q_ASSERT(b <= e);
         Q_ASSERT(e <= where || b > this->end()); // No overlap
         Q_ASSERT(size_t(e - b) <= this->allocatedCapacity() - this->size);
@@ -159,7 +159,7 @@ public:
     void insert(T *where, size_t n, parameter_type t)
     {
         Q_ASSERT(!this->isShared());
-        Q_ASSERT(where >= this->begin() && where < this->end()); // Use copyAppend at end
+        Q_ASSERT(where >= this->begin() && where <= this->end());
         Q_ASSERT(this->allocatedCapacity() - this->size >= n);
 
         ::memmove(static_cast<void *>(where + n), static_cast<void *>(where),
@@ -351,7 +351,7 @@ struct QGenericArrayOps
     {
         Q_ASSERT(this->isMutable());
         Q_ASSERT(!this->isShared());
-        Q_ASSERT(where >= this->begin() && where < this->end()); // Use copyAppend at end
+        Q_ASSERT(where >= this->begin() && where <= this->end());
         Q_ASSERT(b <= e);
         Q_ASSERT(e <= where || b > this->end()); // No overlap
         Q_ASSERT(size_t(e - b) <= this->allocatedCapacity() - this->size);
@@ -388,10 +388,10 @@ struct QGenericArrayOps
         } destroyer(writeIter);
 
         // Construct new elements in array
-        do {
+        while (writeIter != step1End) {
             --readIter, --writeIter;
             new (writeIter) T(*readIter);
-        } while (writeIter != step1End);
+        }
 
         while (writeIter != end) {
             --e, --writeIter;
@@ -450,10 +450,10 @@ struct QGenericArrayOps
         } destroyer(writeIter);
 
         // Construct new elements in array
-        do {
+        while (writeIter != step1End) {
             --readIter, --writeIter;
             new (writeIter) T(*readIter);
-        } while (writeIter != step1End);
+        }
 
         while (writeIter != end) {
             --n, --writeIter;
@@ -551,7 +551,7 @@ struct QMovableArrayOps
     {
         Q_ASSERT(this->isMutable());
         Q_ASSERT(!this->isShared());
-        Q_ASSERT(where >= this->begin() && where < this->end()); // Use copyAppend at end
+        Q_ASSERT(where >= this->begin() && where <= this->end());
         Q_ASSERT(b <= e);
         Q_ASSERT(e <= where || b > this->end()); // No overlap
         Q_ASSERT(size_t(e - b) <= this->allocatedCapacity() - this->size);
diff --git a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
index b503e5e72a8..3f96d7e782a 100644
--- a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
+++ b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
@@ -1373,10 +1373,10 @@ void tst_QArrayData::arrayOpsExtra()
         // empty ranges
         RUN_TEST_FUNC(testInsertRange, intData, 0, intArray.data(), intArray.data());
         RUN_TEST_FUNC(testInsertRange, strData, 0, stringArray.data(), stringArray.data());
-        // RUN_TEST_FUNC(testInsertRange, objData, 0, objArray.data(), objArray.data());  // ### crashes
+        RUN_TEST_FUNC(testInsertRange, objData, 0, objArray.data(), objArray.data());
         RUN_TEST_FUNC(testInsertValue, intData, 1, 0, int());
         RUN_TEST_FUNC(testInsertValue, strData, 1, 0, QString());
-        // RUN_TEST_FUNC(testInsertValue, objData, 1, 0, CountedObject());  // ### crashes
+        RUN_TEST_FUNC(testInsertValue, objData, 1, 0, CountedObject());
 
         // insert at the beginning
         RUN_TEST_FUNC(testInsertRange, intData, 0, intArray.data(), intArray.data() + 1);
@@ -1405,9 +1405,14 @@ void tst_QArrayData::arrayOpsExtra()
         RUN_TEST_FUNC(testInsertValue, strData, strData.size - 3, 3, QLatin1String("foo"));
         RUN_TEST_FUNC(testInsertValue, objData, objData.size - 3, 3, CountedObject());
 
-        // insert at the end (generic and movable operations allow it in value case)
+        // insert at the end
+        RUN_TEST_FUNC(testInsertRange, intData, intData.size, intArray.data(), intArray.data() + 3);
+        RUN_TEST_FUNC(testInsertRange, strData, strData.size, stringArray.data(),
+                      stringArray.data() + 3);
+        RUN_TEST_FUNC(testInsertRange, objData, objData.size, objArray.data(), objArray.data() + 3);
+        RUN_TEST_FUNC(testInsertValue, intData, intData.size, 1, int(-42));
         RUN_TEST_FUNC(testInsertValue, strData, strData.size, 1, QLatin1String("hello, world"));
-        // RUN_TEST_FUNC(testInsertValue, objData, objData.size, 1, CountedObject());  // ### crashes
+        RUN_TEST_FUNC(testInsertValue, objData, objData.size, 1, CountedObject());
     }
 
     // emplace