Fix regression in QVarLengthArray::operator=

There was a serious regression wherei, under certain conditions,
assignment would be treated as an append. This was due to poor tracking
of container invariants inside realloc.

From now on, after the allocation decision, s shall contain the number
of elements in the array to be kept. Deleting extra elements in the old
array needn't update this value. Instead, it needs to be updated once
and if new elements are created afterwards.

Auto-test greatly expanded to avoid future embarassments.

Task-number: QTBUG-10978
Reviewed-by: Olivier Goffart
Reviewed-by: Fabien Freling

Olivier reviewed the patch, Fabien the auto-test.

2 files changed, 304 insertions, 8 deletions

diff --git a/src/corelib/tools/qvarlengtharray.h b/src/corelib/tools/qvarlengtharray.h
index aecb66e525..a70488f006 100644
--- a/src/corelib/tools/qvarlengtharray.h
+++ b/src/corelib/tools/qvarlengtharray.h
@@ -128,9 +128,9 @@ private:
     friend class QPodList<T, Prealloc>;
     void realloc(int size, int alloc);
 
-    int a;
-    int s;
-    T *ptr;
+    int a;      // capacity
+    int s;      // size
+    T *ptr;     // data
     union {
         // ### Qt 5: Use 'Prealloc * sizeof(T)' as array size
         char array[sizeof(qint64) * (((Prealloc * sizeof(T)) / sizeof(qint64)) + 1)];
@@ -193,8 +193,8 @@ Q_OUTOFLINE_TEMPLATE void QVarLengthArray<T, Prealloc>::realloc(int asize, int a
     Q_ASSERT(aalloc >= asize);
     T *oldPtr = ptr;
     int osize = s;
-    // s = asize;
 
+    const int copySize = qMin(asize, osize);
     if (aalloc != a) {
         ptr = reinterpret_cast<T *>(qMalloc(aalloc * sizeof(T)));
         Q_CHECK_PTR(ptr);
@@ -205,7 +205,6 @@ Q_OUTOFLINE_TEMPLATE void QVarLengthArray<T, Prealloc>::realloc(int asize, int a
             if (QTypeInfo<T>::isStatic) {
                 QT_TRY {
                     // copy all the old elements
-                    const int copySize = qMin(asize, osize);
                     while (s < copySize) {
                         new (ptr+s) T(*(oldPtr+s));
                         (oldPtr+s)->~T();
@@ -221,19 +220,19 @@ Q_OUTOFLINE_TEMPLATE void QVarLengthArray<T, Prealloc>::realloc(int asize, int a
                     QT_RETHROW;
                 }
             } else {
-                qMemCopy(ptr, oldPtr, qMin(asize, osize) * sizeof(T));
+                qMemCopy(ptr, oldPtr, copySize * sizeof(T));
             }
         } else {
             ptr = oldPtr;
             return;
         }
     }
+    s = copySize;
 
     if (QTypeInfo<T>::isComplex) {
+        // destroy remaining old objects
         while (osize > asize)
             (oldPtr+(--osize))->~T();
-        if (!QTypeInfo<T>::isStatic)
-            s = osize;
     }
 
     if (oldPtr != reinterpret_cast<T *>(array) && oldPtr != ptr)
diff --git a/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp b/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp
index 1c43069eca..0b7dc131cd 100644
--- a/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp
+++ b/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp
@@ -63,6 +63,7 @@ private slots:
     void oldTests();
     void task214223();
     void QTBUG6718_resize();
+    void QTBUG10978_realloc();
 };
 
 int fooCtor = 0;
@@ -291,5 +292,301 @@ void tst_QVarLengthArray::QTBUG6718_resize()
     }
 }
 
+struct MyBase
+{
+    MyBase()
+        : data(this)
+        , isCopy(false)
+    {
+        ++liveCount;
+    }
+
+    MyBase(MyBase const &)
+        : data(this)
+        , isCopy(true)
+    {
+        ++copyCount;
+        ++liveCount;
+    }
+
+    MyBase & operator=(MyBase const &)
+    {
+        if (!isCopy) {
+            isCopy = true;
+            ++copyCount;
+        } else {
+            ++errorCount;
+        }
+
+        return *this;
+    }
+
+    ~MyBase()
+    {
+        if (isCopy) {
+            if (!copyCount)
+                ++errorCount;
+            else
+                --copyCount;
+        }
+
+        if (!liveCount)
+            ++errorCount;
+        else
+            --liveCount;
+    }
+
+    bool hasMoved() const
+    {
+        return this != data;
+    }
+
+protected:
+    MyBase const * const data;
+    bool isCopy;
+
+public:
+    static int errorCount;
+    static int liveCount;
+    static int copyCount;
+};
+
+int MyBase::errorCount = 0;
+int MyBase::liveCount = 0;
+int MyBase::copyCount = 0;
+
+struct MyPrimitive
+    : MyBase
+{
+    MyPrimitive()
+    {
+        ++errorCount;
+    }
+
+    ~MyPrimitive()
+    {
+        ++errorCount;
+    }
+
+    MyPrimitive(MyPrimitive const &other)
+        : MyBase(other)
+    {
+        ++errorCount;
+    }
+};
+
+Q_DECLARE_TYPEINFO(MyPrimitive, Q_PRIMITIVE_TYPE);
+
+struct MyMovable
+    : MyBase
+{
+};
+
+Q_DECLARE_TYPEINFO(MyMovable, Q_MOVABLE_TYPE);
+
+struct MyComplex
+    : MyBase
+{
+};
+
+Q_DECLARE_TYPEINFO(MyComplex, Q_COMPLEX_TYPE);
+
+
+bool QTBUG10978_proceed = true;
+
+template <class T, int PreAlloc>
+int countMoved(QVarLengthArray<T, PreAlloc> const &c)
+{
+    int result = 0;
+    for (int i = 0; i < c.size(); ++i)
+        if (c[i].hasMoved())
+            ++result;
+
+    return result;
+}
+
+template <class T>
+void QTBUG10978_test()
+{
+    QTBUG10978_proceed = false;
+
+    typedef QVarLengthArray<T, 16> Container;
+    enum {
+        isStatic = QTypeInfo<T>::isStatic,
+        isComplex = QTypeInfo<T>::isComplex,
+
+        isPrimitive = !isComplex && !isStatic,
+        isMovable = !isStatic
+    };
+
+    // Constructors
+    Container a;
+    QCOMPARE( MyBase::liveCount, 0 );
+    QCOMPARE( MyBase::copyCount, 0 );
+
+    QVERIFY( a.capacity() >= 16 );
+    QCOMPARE( a.size(), 0 );
+
+    Container b_real(8);
+    Container const &b = b_real;
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 8 );
+    QCOMPARE( MyBase::copyCount, 0 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // Assignment
+    a = b;
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 16 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 8 : 0 );
+    QVERIFY( a.capacity() >= 16 );
+    QCOMPARE( a.size(), 8 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // append
+    a.append(b.data(), b.size());
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 24 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 16 : 0 );
+
+    QVERIFY( a.capacity() >= 16 );
+    QCOMPARE( a.size(), 16 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // removeLast
+    a.removeLast();
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 23 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 15 : 0 );
+
+    QVERIFY( a.capacity() >= 16 );
+    QCOMPARE( a.size(), 15 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // Movable types
+    const int capacity = a.capacity();
+    if (!isPrimitive)
+        QCOMPARE( countMoved(a), 0 );
+
+    // Reserve, no re-allocation
+    a.reserve(capacity);
+    if (!isPrimitive)
+        QCOMPARE( countMoved(a), 0 );
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 23 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 15 : 0 );
+
+    QCOMPARE( a.capacity(), capacity );
+    QCOMPARE( a.size(), 15 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // Reserve, force re-allocation
+    a.reserve(capacity * 2);
+    if (!isPrimitive)
+        QCOMPARE( countMoved(a), isMovable ? 15 : 0 );
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 23 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 15 : 0 );
+
+    QVERIFY( a.capacity() >= capacity * 2 );
+    QCOMPARE( a.size(), 15 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // resize, grow
+    a.resize(40);
+    if (!isPrimitive)
+        QCOMPARE( countMoved(a), isMovable ? 15 : 0 );
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 48 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 15 : 0 );
+
+    QVERIFY( a.capacity() >= a.size() );
+    QCOMPARE( a.size(), 40 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // Copy constructor, allocate
+    {
+        Container c(a);
+        if (!isPrimitive)
+            QCOMPARE( countMoved(c), 0 );
+        QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 88 );
+        QCOMPARE( MyBase::copyCount, isComplex ? 55 : 0 );
+
+        QVERIFY( a.capacity() >= a.size() );
+        QCOMPARE( a.size(), 40 );
+
+        QVERIFY( b.capacity() >= 16 );
+        QCOMPARE( b.size(), 8 );
+
+        QVERIFY( c.capacity() >= 40 );
+        QCOMPARE( c.size(), 40 );
+    }
+
+    // resize, shrink
+    a.resize(10);
+    if (!isPrimitive)
+        QCOMPARE( countMoved(a), isMovable ? 10 : 0 );
+    QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 18 );
+    QCOMPARE( MyBase::copyCount, isComplex ? 10 : 0 );
+
+    QVERIFY( a.capacity() >= a.size() );
+    QCOMPARE( a.size(), 10 );
+
+    QVERIFY( b.capacity() >= 16 );
+    QCOMPARE( b.size(), 8 );
+
+    // Copy constructor, don't allocate
+    {
+        Container c(a);
+        if (!isPrimitive)
+            QCOMPARE( countMoved(c), 0 );
+        QCOMPARE( MyBase::liveCount, isPrimitive ? 0 : 28 );
+        QCOMPARE( MyBase::copyCount, isComplex ? 20 : 0 );
+
+        QVERIFY( a.capacity() >= a.size() );
+        QCOMPARE( a.size(), 10 );
+
+        QVERIFY( b.capacity() >= 16 );
+        QCOMPARE( b.size(), 8 );
+
+        QVERIFY( c.capacity() >= 16 );
+        QCOMPARE( c.size(), 10 );
+    }
+
+    a.clear();
+    QCOMPARE( a.size(), 0 );
+
+    b_real.clear();
+    QCOMPARE( b.size(), 0 );
+
+    QCOMPARE(MyBase::errorCount, 0);
+    QCOMPARE(MyBase::liveCount, 0);
+
+    // All done
+    QTBUG10978_proceed = true;
+}
+
+void tst_QVarLengthArray::QTBUG10978_realloc()
+{
+    QTBUG10978_test<MyBase>();
+    QVERIFY(QTBUG10978_proceed);
+
+    QTBUG10978_test<MyPrimitive>();
+    QVERIFY(QTBUG10978_proceed);
+
+    QTBUG10978_test<MyMovable>();
+    QVERIFY(QTBUG10978_proceed);
+
+    QTBUG10978_test<MyComplex>();
+    QVERIFY(QTBUG10978_proceed);
+}
+
 QTEST_APPLESS_MAIN(tst_QVarLengthArray)
 #include "tst_qvarlengtharray.moc"