Don't operate on bogus data, assert on preconditions instead

QVector::erase shouldn't try to make sense of iterators it doesn't own,
so the validation being done here is bogus and dangerous. Instead, it's
preferrable to assert, the user needs to ensure proper ownership.

The case of erasing an empty sequence is not checked for preconditions
to allow

    QVector v;
    v.erase(v.begin(), v.end());

, while being stricter on other uses.

Autotests were using ill-formed calls to the single argument erase()
function on an empty vector and were fixed. This function erases exactly
one element, the one pointed to by abegin and require the element exist
and be valid.

Change-Id: I5f1a6d0d8da072eae0c73a3012620c4ce1065cf0
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

diff --git a/src/corelib/tools/qvector.h b/src/corelib/tools/qvector.h
index e2cb7fb..b75c297 100644 (file)
--- a/src/corelib/tools/qvector.h
+++ b/src/corelib/tools/qvector.h
@@ -581,14 +581,15 @@ typename QVector<T>::iterator QVector<T>::insert(iterator before, size_type n, c
 template <typename T>
 typename QVector<T>::iterator QVector<T>::erase(iterator abegin, iterator aend)
 {
-    if (abegin < d->begin())
-        abegin = d->begin();
-    if (aend > d->end())
-        aend = d->end();
+    const int itemsToErase = aend - abegin;
+
+    if (!itemsToErase)
+        return abegin;
 
+    Q_ASSERT(abegin >= d->begin());
+    Q_ASSERT(aend <= d->end());
     Q_ASSERT(abegin <= aend);
 
-    const int itemsToErase = aend - abegin;
     const int itemsUntouched = abegin - d->begin();
 
     // FIXME we could do a proper realloc, which copy constructs only needed data.

diff --git a/tests/auto/corelib/tools/qvector/tst_qvector.cpp b/tests/auto/corelib/tools/qvector/tst_qvector.cpp
index 56570b8..09d3051 100644 (file)
--- a/tests/auto/corelib/tools/qvector/tst_qvector.cpp
+++ b/tests/auto/corelib/tools/qvector/tst_qvector.cpp
@@ -796,16 +796,12 @@ void tst_QVector::eraseEmpty() const
 {
     {
         QVector<T> v;
-        v.erase(v.begin());
-        QCOMPARE(v.size(), 0);
         v.erase(v.begin(), v.end());
         QCOMPARE(v.size(), 0);
     }
     {
         QVector<T> v;
         v.setSharable(false);
-        v.erase(v.begin());
-        QCOMPARE(v.size(), 0);
         v.erase(v.begin(), v.end());
         QCOMPARE(v.size(), 0);
     }
@@ -836,8 +832,6 @@ void tst_QVector::eraseEmptyReserved() const
     {
         QVector<T> v;
         v.reserve(10);
-        v.erase(v.begin());
-        QCOMPARE(v.size(), 0);
         v.erase(v.begin(), v.end());
         QCOMPARE(v.size(), 0);
     }
@@ -845,8 +839,6 @@ void tst_QVector::eraseEmptyReserved() const
         QVector<T> v;
         v.reserve(10);
         v.setSharable(false);
-        v.erase(v.begin());
-        QCOMPARE(v.size(), 0);
         v.erase(v.begin(), v.end());
         QCOMPARE(v.size(), 0);
     }