From 8060094144d6104659b8ce3b88d6f8b1e53cfb59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Abecasis?= Date: Wed, 26 May 2010 20:04:57 +0200 Subject: 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. --- src/corelib/tools/qvarlengtharray.h | 15 +- tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp | 297 +++++++++++++++++++++ 2 files changed, 304 insertions(+), 8 deletions(-) diff --git a/src/corelib/tools/qvarlengtharray.h b/src/corelib/tools/qvarlengtharray.h index aecb66e..a70488f 100644 --- a/src/corelib/tools/qvarlengtharray.h +++ b/src/corelib/tools/qvarlengtharray.h @@ -128,9 +128,9 @@ private: friend class QPodList; 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::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(qMalloc(aalloc * sizeof(T))); Q_CHECK_PTR(ptr); @@ -205,7 +205,6 @@ Q_OUTOFLINE_TEMPLATE void QVarLengthArray::realloc(int asize, int a if (QTypeInfo::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::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::isComplex) { + // destroy remaining old objects while (osize > asize) (oldPtr+(--osize))->~T(); - if (!QTypeInfo::isStatic) - s = osize; } if (oldPtr != reinterpret_cast(array) && oldPtr != ptr) diff --git a/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp b/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp index 1c43069..0b7dc13 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 +int countMoved(QVarLengthArray const &c) +{ + int result = 0; + for (int i = 0; i < c.size(); ++i) + if (c[i].hasMoved()) + ++result; + + return result; +} + +template +void QTBUG10978_test() +{ + QTBUG10978_proceed = false; + + typedef QVarLengthArray Container; + enum { + isStatic = QTypeInfo::isStatic, + isComplex = QTypeInfo::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(); + QVERIFY(QTBUG10978_proceed); + + QTBUG10978_test(); + QVERIFY(QTBUG10978_proceed); + + QTBUG10978_test(); + QVERIFY(QTBUG10978_proceed); + + QTBUG10978_test(); + QVERIFY(QTBUG10978_proceed); +} + QTEST_APPLESS_MAIN(tst_QVarLengthArray) #include "tst_qvarlengtharray.moc" -- cgit v0.12 From 7249e71c722739f35641df35c70a1a5fc5e24a41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Abecasis?= Date: Thu, 27 May 2010 00:39:48 +0200 Subject: Allow auto-test to compile when using namespaces Auto-test added in 8060094144d6104659b8ce3b88d6f8b1e53cfb59 does not compile when using Qt in namespace, because Q_DECLARE_TYPEINFO needs to be used from inside QT_NAMESPACE. Wrapper macros added. Task-number: QTBUG-10978 --- tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp b/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp index 0b7dc13..39805e1 100644 --- a/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp +++ b/tests/auto/qvarlengtharray/tst_qvarlengtharray.cpp @@ -375,22 +375,23 @@ struct MyPrimitive } }; -Q_DECLARE_TYPEINFO(MyPrimitive, Q_PRIMITIVE_TYPE); - struct MyMovable : MyBase { }; -Q_DECLARE_TYPEINFO(MyMovable, Q_MOVABLE_TYPE); - struct MyComplex : MyBase { }; +QT_BEGIN_NAMESPACE + +Q_DECLARE_TYPEINFO(MyPrimitive, Q_PRIMITIVE_TYPE); +Q_DECLARE_TYPEINFO(MyMovable, Q_MOVABLE_TYPE); Q_DECLARE_TYPEINFO(MyComplex, Q_COMPLEX_TYPE); +QT_END_NAMESPACE bool QTBUG10978_proceed = true; -- cgit v0.12