From bdc115d9698d4f3cf5ce089dbddca75e425d3eeb Mon Sep 17 00:00:00 2001 From: Andreas Hartmetz Date: Sun, 14 Oct 2012 04:28:51 +0200 Subject: detach() safely in QVector::erase(), and update callers to not detach. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove() can use non-detaching iterators internally before calling erase(), which hasn't been exploited so far, so that the detach() in erase() never actually detached. When using erase() from outside, you can't do it legally without calling begin() or end() that detach() before erase() is called. Now remove() doesn't detach anymore, and detaching in erase() works. With new tests that fail after changing only the erase() callers and pass again after fixing erase(). Change-Id: I47c0a9e362dce8628ec566f5437d951755de96c8 Reviewed-by: Thorbjørn Lund Martsum Reviewed-by: Olivier Goffart Reviewed-by: Konstantin Ritt --- src/corelib/tools/qvector.h | 10 +- tests/auto/corelib/tools/qvector/tst_qvector.cpp | 117 ++++++++++++++++++++--- 2 files changed, 110 insertions(+), 17 deletions(-) diff --git a/src/corelib/tools/qvector.h b/src/corelib/tools/qvector.h index 925ad17110..94733f77da 100644 --- a/src/corelib/tools/qvector.h +++ b/src/corelib/tools/qvector.h @@ -200,8 +200,8 @@ public: typedef int size_type; inline void push_back(const T &t) { append(t); } inline void push_front(const T &t) { prepend(t); } - void pop_back() { Q_ASSERT(!isEmpty()); erase(end()-1); } - void pop_front() { Q_ASSERT(!isEmpty()); erase(begin()); } + void pop_back() { Q_ASSERT(!isEmpty()); erase(d->end() - 1); } + void pop_front() { Q_ASSERT(!isEmpty()); erase(d->begin()); } inline bool empty() const { return d->size == 0; } inline T& front() { return first(); } @@ -366,11 +366,11 @@ inline void QVector::insert(int i, int n, const T &t) template inline void QVector::remove(int i, int n) { Q_ASSERT_X(i >= 0 && n >= 0 && i + n <= d->size, "QVector::remove", "index out of range"); - erase(begin() + i, begin() + i + n); } + erase(d->begin() + i, d->begin() + i + n); } template inline void QVector::remove(int i) { Q_ASSERT_X(i >= 0 && i < d->size, "QVector::remove", "index out of range"); - erase(begin() + i, begin() + i + 1); } + erase(d->begin() + i, d->begin() + i + 1); } template inline void QVector::prepend(const T &t) { insert(begin(), 1, t); } @@ -607,6 +607,8 @@ typename QVector::iterator QVector::erase(iterator abegin, iterator aend) // FIXME we ara about to delete data maybe it is good time to shrink? if (d->alloc) { detach(); + abegin = d->begin() + itemsUntouched; + aend = abegin + itemsToErase; if (QTypeInfo::isStatic) { iterator moveBegin = abegin + itemsToErase; iterator moveEnd = d->end(); diff --git a/tests/auto/corelib/tools/qvector/tst_qvector.cpp b/tests/auto/corelib/tools/qvector/tst_qvector.cpp index 6304477ba3..0eda837644 100644 --- a/tests/auto/corelib/tools/qvector/tst_qvector.cpp +++ b/tests/auto/corelib/tools/qvector/tst_qvector.cpp @@ -205,8 +205,11 @@ private slots: void eraseEmptyReservedMovable() const; void eraseEmptyReservedCustom() const; void eraseInt() const; + void eraseIntShared() const; void eraseMovable() const; + void eraseMovableShared() const; void eraseCustom() const; + void eraseCustomShared() const; void eraseReservedInt() const; void eraseReservedMovable() const; void eraseReservedCustom() const; @@ -268,6 +271,7 @@ private slots: void detachInt() const; void detachMovable() const; void detachCustom() const; + private: template void copyConstructor() const; template void add() const; @@ -278,7 +282,7 @@ private: template void empty() const; template void eraseEmpty() const; template void eraseEmptyReserved() const; - template void erase() const; + template void erase(bool shared) const; template void eraseReserved() const; template void fill() const; template void fromList() const; @@ -300,6 +304,15 @@ template struct SimpleValue { return Values[index % MaxIndex]; } + + static QVector vector(int size) + { + QVector ret; + for (int i = 0; i < size; i++) + ret.append(at(i)); + return ret; + } + static const uint MaxIndex = 6; static const T Values[MaxIndex]; }; @@ -565,7 +578,6 @@ void tst_QVector::capacity() const // make sure it grows ok myvec << SimpleValue::at(0) << SimpleValue::at(1) << SimpleValue::at(2); QVERIFY(myvec.capacity() >= 6); - // let's try squeeze a bit myvec.remove(3); myvec.remove(3); @@ -865,60 +877,139 @@ void tst_QVector::eraseEmptyReservedCustom() const } template -void tst_QVector::erase() const +struct SharedVectorChecker { + SharedVectorChecker(const QVector &original, bool doCopyVector) + : originalSize(-1), + copy(0) { - QVector v(12); + if (doCopyVector) { + originalSize = original.size(); + copy = new QVector(original); + // this is unlikely to fail, but if the check in the destructor fails it's good to know that + // we were still alright here. + QCOMPARE(originalSize, copy->size()); + } + } + + ~SharedVectorChecker() + { + if (copy) + QCOMPARE(copy->size(), originalSize); + delete copy; + } + + int originalSize; + QVector *copy; +}; + +template +void tst_QVector::erase(bool shared) const +{ + // note: remove() is actually more efficient, and more dangerous, because it uses the non-detaching + // begin() / end() internally. you can also use constBegin() and constEnd() with erase(), but only + // using reinterpret_cast... because both iterator types are really just pointers. + // so we use a mix of erase() and remove() to cover more cases. + { + QVector v = SimpleValue::vector(12); + SharedVectorChecker svc(v, shared); v.erase(v.begin()); QCOMPARE(v.size(), 11); + for (int i = 0; i < 11; i++) + QCOMPARE(v.at(i), SimpleValue::at(i + 1)); v.erase(v.begin(), v.end()); QCOMPARE(v.size(), 0); + if (shared) + QCOMPARE(SimpleValue::vector(12), *svc.copy); } { - QVector v(12); - v.erase(v.begin() + 1); + QVector v = SimpleValue::vector(12); + SharedVectorChecker svc(v, shared); + v.remove(1); QCOMPARE(v.size(), 11); + QCOMPARE(v.at(0), SimpleValue::at(0)); + for (int i = 1; i < 11; i++) + QCOMPARE(v.at(i), SimpleValue::at(i + 1)); v.erase(v.begin() + 1, v.end()); QCOMPARE(v.size(), 1); + QCOMPARE(v.at(0), SimpleValue::at(0)); + if (shared) + QCOMPARE(SimpleValue::vector(12), *svc.copy); } { - QVector v(12); + QVector v = SimpleValue::vector(12); + SharedVectorChecker svc(v, shared); v.erase(v.begin(), v.end() - 1); QCOMPARE(v.size(), 1); + QCOMPARE(v.at(0), SimpleValue::at(11)); + if (shared) + QCOMPARE(SimpleValue::vector(12), *svc.copy); } { - QVector v(12); - v.erase(v.begin() + 5); + QVector v = SimpleValue::vector(12); + SharedVectorChecker svc(v, shared); + v.remove(5); QCOMPARE(v.size(), 11); + for (int i = 0; i < 5; i++) + QCOMPARE(v.at(i), SimpleValue::at(i)); + for (int i = 5; i < 11; i++) + QCOMPARE(v.at(i), SimpleValue::at(i + 1)); v.erase(v.begin() + 1, v.end() - 1); + QCOMPARE(v.at(0), SimpleValue::at(0)); + QCOMPARE(v.at(1), SimpleValue::at(11)); QCOMPARE(v.size(), 2); + if (shared) + QCOMPARE(SimpleValue::vector(12), *svc.copy); } { - QVector v(10); + QVector v = SimpleValue::vector(10); + SharedVectorChecker svc(v, shared); v.setSharable(false); + SharedVectorChecker svc2(v, shared); v.erase(v.begin() + 3); QCOMPARE(v.size(), 9); v.erase(v.begin(), v.end() - 1); QCOMPARE(v.size(), 1); + if (shared) + QCOMPARE(SimpleValue::vector(10), *svc.copy); } } void tst_QVector::eraseInt() const { - erase(); + erase(false); +} + +void tst_QVector::eraseIntShared() const +{ + erase(true); } void tst_QVector::eraseMovable() const { const int instancesCount = Movable::counter; - erase(); + erase(false); + QCOMPARE(instancesCount, Movable::counter); +} + +void tst_QVector::eraseMovableShared() const +{ + const int instancesCount = Movable::counter; + erase(true); QCOMPARE(instancesCount, Movable::counter); } void tst_QVector::eraseCustom() const { const int instancesCount = Custom::counter; - erase(); + erase(false); + QCOMPARE(instancesCount, Custom::counter); +} + +void tst_QVector::eraseCustomShared() const +{ + const int instancesCount = Custom::counter; + erase(true); QCOMPARE(instancesCount, Custom::counter); } -- cgit v1.2.3