From bf3e80023a70b86c2bff793cb8dd86055236ddeb Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 3 Apr 2014 15:44:59 -0700 Subject: [PATCH] Stop using setSharable in the Java-style mutable iterators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First and foremost, the STL-style iterators don't do this. Those don't provide a guarantee that the container won't get shared again while the iterator is active. Second, there's no protection against a second mutable iterator being created and resetting the sharable flag back to true. [ChangeLog][Important behavior changes] The mutable Java-style iterators like QListMutableIterator and QHashMutableIterator no longer set the parent container to unsharable mode. If you create a copy of the container being iterated on after the iterator, any changes done with the iterator might affect the copy too. Discussed-on: http://lists.qt-project.org/pipermail/development/2014-February/015724.html Change-Id: Iccfe411d5558c85ae459cff944215614c392388e Reviewed-by: Jędrzej Nowacki --- src/corelib/tools/qiterator.h | 13 +-- src/corelib/tools/qiterator.qdoc | 20 ---- src/corelib/tools/qset.h | 7 +- .../tools/collections/tst_collections.cpp | 92 +------------------ 4 files changed, 7 insertions(+), 125 deletions(-) diff --git a/src/corelib/tools/qiterator.h b/src/corelib/tools/qiterator.h index 3b86edd750..86b78eb522 100644 --- a/src/corelib/tools/qiterator.h +++ b/src/corelib/tools/qiterator.h @@ -87,12 +87,9 @@ class QMutable##C##Iterator \ public: \ inline QMutable##C##Iterator(Q##C &container) \ : c(&container) \ - { c->setSharable(false); i = c->begin(); n = c->end(); } \ - inline ~QMutable##C##Iterator() \ - { c->setSharable(true); } \ + { i = c->begin(); n = c->end(); } \ inline QMutable##C##Iterator &operator=(Q##C &container) \ - { c->setSharable(true); c = &container; c->setSharable(false); \ - i = c->begin(); n = c->end(); return *this; } \ + { c = &container; i = c->begin(); n = c->end(); return *this; } \ inline void toFront() { i = c->begin(); n = c->end(); } \ inline void toBack() { i = c->end(); n = i; } \ inline bool hasNext() const { return c->constEnd() != const_iterator(i); } \ @@ -160,11 +157,9 @@ class QMutable##C##Iterator \ public: \ inline QMutable##C##Iterator(Q##C &container) \ : c(&container) \ - { c->setSharable(false); i = c->begin(); n = c->end(); } \ - inline ~QMutable##C##Iterator() \ - { c->setSharable(true); } \ + { i = c->begin(); n = c->end(); } \ inline QMutable##C##Iterator &operator=(Q##C &container) \ - { c->setSharable(true); c = &container; c->setSharable(false); i = c->begin(); n = c->end(); return *this; } \ + { c = &container; i = c->begin(); n = c->end(); return *this; } \ inline void toFront() { i = c->begin(); n = c->end(); } \ inline void toBack() { i = c->end(); n = c->end(); } \ inline bool hasNext() const { return const_iterator(i) != c->constEnd(); } \ diff --git a/src/corelib/tools/qiterator.qdoc b/src/corelib/tools/qiterator.qdoc index 1ae9c58daa..f140af152d 100644 --- a/src/corelib/tools/qiterator.qdoc +++ b/src/corelib/tools/qiterator.qdoc @@ -501,17 +501,6 @@ \sa operator=() */ -/*! - \fn QMutableListIterator::~QMutableListIterator() - \fn QMutableLinkedListIterator::~QMutableLinkedListIterator() - \fn QMutableVectorIterator::~QMutableVectorIterator() - \fn QMutableSetIterator::~QMutableSetIterator() - - Destroys the iterator. - - \sa operator=() -*/ - /*! \fn QMutableListIterator &QMutableListIterator::operator=(QList &list) \fn QMutableLinkedListIterator &QMutableLinkedListIterator::operator=(QLinkedList &list) \fn QListIterator &QListIterator::operator=(const QList &list) @@ -1121,15 +1110,6 @@ \sa operator=() */ -/*! - \fn QMutableMapIterator::~QMutableMapIterator() - \fn QMutableHashIterator::~QMutableHashIterator() - - Destroys the iterator. - - \sa operator=() -*/ - /*! \fn QMapIterator &QMapIterator::operator=(const QMap &map) \fn QMutableMapIterator &QMutableMapIterator::operator=(QMap &map) diff --git a/src/corelib/tools/qset.h b/src/corelib/tools/qset.h index 992243def6..9682c97fec 100644 --- a/src/corelib/tools/qset.h +++ b/src/corelib/tools/qset.h @@ -356,12 +356,9 @@ class QMutableSetIterator public: inline QMutableSetIterator(QSet &container) : c(&container) - { c->setSharable(false); i = c->begin(); n = c->end(); } - inline ~QMutableSetIterator() - { c->setSharable(true); } + { i = c->begin(); n = c->end(); } inline QMutableSetIterator &operator=(QSet &container) - { c->setSharable(true); c = &container; c->setSharable(false); - i = c->begin(); n = c->end(); return *this; } + { c = &container; i = c->begin(); n = c->end(); return *this; } inline void toFront() { i = c->begin(); n = c->end(); } inline void toBack() { i = c->end(); n = i; } inline bool hasNext() const { return c->constEnd() != i; } diff --git a/tests/auto/corelib/tools/collections/tst_collections.cpp b/tests/auto/corelib/tools/collections/tst_collections.cpp index 94b739aa54..33aa1c4888 100644 --- a/tests/auto/corelib/tools/collections/tst_collections.cpp +++ b/tests/auto/corelib/tools/collections/tst_collections.cpp @@ -2437,8 +2437,7 @@ void testContainer() } /* - Verify that the 'sharable' flag is true while no mutable - iterator is active. + Verify that the 'sharable' flag is true in populated containers. */ { Container c1; @@ -2453,95 +2452,6 @@ void testContainer() QVERIFY(!c2.isDetached()); } - /* - Verify that the 'sharable' flag is set to false by the - mutable iterator. - */ - { - Container c1; - populate(c1); - QVERIFY(c1.size() == 4); - QVERIFY(c1.isDetached()); - - ContainerMutableIterator i(c1); - i.next(); - - Container c2 = c1; - QVERIFY(c1.size() == 4); - QVERIFY(c2.size() == 4); - QVERIFY(c1.isDetached()); - QVERIFY(c2.isDetached()); - - i.remove(); - QVERIFY(c1.size() == 3); - QVERIFY(c2.size() == 4); - } - - /* - Verify that the 'sharable' flag is reset to true by the - mutable iterator's destructor. - */ - { - Container c1; - populate(c1); - QVERIFY(c1.size() == 4); - QVERIFY(c1.isDetached()); - - { - ContainerMutableIterator i(c1); - i.next(); - } - - Container c2 = c1; - QVERIFY(c1.size() == 4); - QVERIFY(c2.size() == 4); - QVERIFY(!c1.isDetached()); - QVERIFY(!c2.isDetached()); - } - - /* - Verify that the 'sharable' flag only affects the original - object, not the copies. - */ - { - Container c1; - populate(c1); - QVERIFY(c1.size() == 4); - QVERIFY(c1.isDetached()); - - Container c2 = c1; - QVERIFY(isSharable(c2)); - - ContainerMutableIterator i(c1); - QVERIFY(!isSharable(c1)); - QVERIFY(isSharable(c2)); - - Container c3 = c1; - QVERIFY(!isSharable(c1)); - QVERIFY(isSharable(c2)); - QVERIFY(isSharable(c3)); - QVERIFY(c1.isDetached()); - QVERIFY(c2.isDetached()); - QVERIFY(c3.isDetached()); - - Container c4; - c4 = c1; - QVERIFY(!isSharable(c1)); - QVERIFY(isSharable(c2)); - QVERIFY(isSharable(c4)); - QVERIFY(c1.isDetached()); - QVERIFY(c2.isDetached()); - QVERIFY(c4.isDetached()); - - c3 = c2; - QVERIFY(!isSharable(c1)); - QVERIFY(isSharable(c2)); - QVERIFY(isSharable(c3)); - QVERIFY(c1.isDetached()); - QVERIFY(!c2.isDetached()); - QVERIFY(!c3.isDetached()); - } - /* test that the move operators work properly */ { Container c1 = Container(newInstance());