Replace QScriptBagContainer by QIntrusiveList

QIntrusiveList effectively does the same as
QScriptBagContainer, without the need to inherit a
Node class. Let's avoid the code duplication.

However, QIntrusiveList::remove() and insert()
are less strict than QScriptBagContainer; introduce
QScriptIntrusiveList that performs the same sanity
checks (no duplicate insertion, etc). Ideally these
checks would be merged into QIntrusiveList.

Also rename QJSValuePrivate::reinitialize() to
invalidate(), and make it not call
engine->unregisterValue(this) any more. Values are
only invalidated at engine destruction time, and
it's the engine's responsibility to erase the list.

Change-Id: I60fc61ee8f90a716a285b1dd1bf4d6a08a9349df
Reviewed-on: http://codereview.qt.nokia.com/2628
Reviewed-by: Qt Sanity Bot <qt_sanity_bot@ovi.com>
Reviewed-by: Jędrzej Nowacki <jedrzej.nowacki@nokia.com>

6 files changed, 49 insertions, 184 deletions

diff --git a/src/declarative/qml/v8/qjsvalue_impl_p.h b/src/declarative/qml/v8/qjsvalue_impl_p.h
index 39bfd898ee..b0ad7669d7 100644
--- a/src/declarative/qml/v8/qjsvalue_impl_p.h
+++ b/src/declarative/qml/v8/qjsvalue_impl_p.h
@@ -1038,12 +1038,14 @@ bool QJSValuePrivate::assignEngine(QV8Engine* engine)
 
 /*!
   \internal
-  reinitialize this value to an invalid value.
+  Invalidates this value.
+
+  Does not remove the value from the engine's list of
+  registered values; that's the responsibility of the caller.
 */
-void QJSValuePrivate::reinitialize()
+void QJSValuePrivate::invalidate()
 {
     if (isJSBased()) {
-        m_engine->unregisterValue(this);
         m_value.Dispose();
         m_value.Clear();
     } else if (isStringBased()) {
diff --git a/src/declarative/qml/v8/qjsvalue_p.h b/src/declarative/qml/v8/qjsvalue_p.h
index b50f573b5c..09cd38b0bf 100644
--- a/src/declarative/qml/v8/qjsvalue_p.h
+++ b/src/declarative/qml/v8/qjsvalue_p.h
@@ -43,7 +43,7 @@
 #include <QtCore/qvarlengtharray.h>
 #include <qdebug.h>
 
-#include "qscripttools_p.h"
+#include <private/qintrusivelist_p.h>
 #include "qscriptshareddata_p.h"
 #include "qjsvalue.h"
 
@@ -57,7 +57,6 @@ class QV8Engine;
 */
 class QJSValuePrivate
         : public QSharedData
-        , public QScriptLinkedNode
 {
 public:
     inline QJSValuePrivate();
@@ -81,7 +80,7 @@ public:
     inline QJSValuePrivate(QV8Engine *engine, const QString& value);
     inline QJSValuePrivate(QV8Engine *engine, QJSValue::SpecialValue value);
     inline QJSValuePrivate(QV8Engine *engine, v8::Handle<v8::Value>);
-    inline void reinitialize();
+    inline void invalidate();
 
     inline bool toBool() const;
     inline double toNumber() const;
@@ -148,6 +147,7 @@ public:
     inline operator v8::Handle<v8::Object>() const;
     inline v8::Handle<v8::Value> asV8Value(QV8Engine *engine);
 private:
+    QIntrusiveListNode m_node;
     QV8Engine *m_engine;
 
     // Please, update class documentation when you change the enum.
@@ -183,6 +183,8 @@ private:
     inline bool isNumberBased() const;
     inline bool isStringBased() const;
     inline bool prepareArgumentsForCall(v8::Handle<v8::Value> argv[], const QJSValueList& arguments) const;
+
+    friend class QV8Engine;
 };
 
 QT_END_NAMESPACE
diff --git a/src/declarative/qml/v8/qjsvalueiterator_p.h b/src/declarative/qml/v8/qjsvalueiterator_p.h
index d3033d496b..6113d3dd4f 100644
--- a/src/declarative/qml/v8/qjsvalueiterator_p.h
+++ b/src/declarative/qml/v8/qjsvalueiterator_p.h
@@ -24,16 +24,17 @@
 #ifndef QJSVALUEITERATOR_P_H
 #define QJSVALUEITERATOR_P_H
 
-#include <qscripttools_p.h>
-#include <qjsvalue_p.h>
+#include <private/qintrusivelist_p.h>
+#include "qjsvalue_p.h"
 
-#include <qv8_p.h>
+#include <private/qv8_p.h>
 
 QT_BEGIN_NAMESPACE
 
 class QV8Engine;
 
-class QJSValueIteratorPrivate : public QScriptLinkedNode {
+class QJSValueIteratorPrivate
+{
 public:
     inline QJSValueIteratorPrivate(const QJSValuePrivate* value);
     inline ~QJSValueIteratorPrivate();
@@ -52,10 +53,13 @@ public:
 private:
     Q_DISABLE_COPY(QJSValueIteratorPrivate)
 
+    QIntrusiveListNode m_node;
     QScriptSharedDataPointer<QJSValuePrivate> m_object;
     v8::Persistent<v8::Array> m_names;
     uint32_t m_index;
     uint32_t m_count;
+
+    friend class QV8Engine;
 };
 
 
diff --git a/src/declarative/qml/v8/qscripttools_p.h b/src/declarative/qml/v8/qscripttools_p.h
index f74fbab83f..c8dace0b9f 100644
--- a/src/declarative/qml/v8/qscripttools_p.h
+++ b/src/declarative/qml/v8/qscripttools_p.h
@@ -36,180 +36,32 @@
 #ifndef QSCRIPTTOOLS_P_H
 #define QSCRIPTTOOLS_P_H
 
-#include <qdebug.h>
+#include <private/qintrusivelist_p.h>
 
 QT_BEGIN_NAMESPACE
 
-template<class T>
-class QScriptBagContainer;
-
-/*!
-  \internal
-  \interface
-  Helper class for a container. The purpuse of it is to add two pointer properties to a class
-  inheriting this class without bloating an interface.
-
-  This class exist only as a memory storage implementation. The only way to use it is to inherit it.
-*/
-class QScriptLinkedNode
+template<class N, QIntrusiveListNode N::*member>
+class QScriptIntrusiveList : public QIntrusiveList<N, member>
 {
-protected:
-    QScriptLinkedNode()
-        : m_next(0)
-        , m_prev(0)
-    {}
-
-    ~QScriptLinkedNode()
-    {
-        Q_ASSERT_X(!isUsed(), Q_FUNC_INFO, "Destorying QScriptLinkedNode instance that still is in a container");
-    }
-
-private:
-    bool isUsed() const
-    {
-        return m_next || m_prev;
-    }
-
-#if defined(Q_NO_TEMPLATE_FRIENDS)
 public:
-#else
-    template<class T>
-    friend class QScriptBagContainer;
-#endif
-    QScriptLinkedNode *m_next;
-    QScriptLinkedNode *m_prev;
+    inline void insert(N *n);
+    inline void remove(N *n);
 };
 
-/*!
-  \internal
-  The QScriptBagContainer is a simple, low level, set like container for a pointer type castable to
-  QScriptLinkedNode*.
-  Algorithms complexity:
-  put: O(1)
-  get: O(1)
-  forEach: O(n)
-  \note This container doesn't take ownership of pointed values.
-  \attention All values have to be unique.
-*/
-template<class T>
-class QScriptBagContainer
+template<class N, QIntrusiveListNode N::*member>
+void QScriptIntrusiveList<N, member>::insert(N *n)
 {
-public:
-    QScriptBagContainer()
-        : m_first(0)
-    {}
-
-    /*!
-      \internal
-      Add a this \a value to this container
-    */
-    void insert(T* value)
-    {
-        //dump(Q_FUNC_INFO, value);
-        Q_ASSERT_X(!contains(value), Q_FUNC_INFO, "Can't insert a value which is in the bag already");
-        QScriptLinkedNode* v = static_cast<QScriptLinkedNode*>(value);
-        Q_ASSERT(v);
-        Q_ASSERT_X(!v->m_next && !v->m_prev, Q_FUNC_INFO, "Can't insert a value which is in an another bag");
-
-        if (m_first)
-            m_first->m_prev = v;
-
-        v->m_next = m_first;
-        v->m_prev = 0;
-        m_first = v;
-    }
-
-    /*!
-      \internal
-      Remove this \a value from this container
-    */
-    void remove(T* value)
-    {
-        //dump(Q_FUNC_INFO, value);
-        QScriptLinkedNode* v = static_cast<QScriptLinkedNode*>(value);
-        Q_ASSERT(v);
+    Q_ASSERT_X(!contains(n), Q_FUNC_INFO, "Can't insert a value which is in the list already");
+    Q_ASSERT_X(!(n->*member).isInList(), Q_FUNC_INFO, "Can't insert a value which is in another list");
+    QIntrusiveList<N, member>::insert(n);
+}
 
-        if (!v->m_next && !v->m_prev && m_first != v) {
-            // ignore that value as it is not registered at all
-            // FIXME: That may be optimized out if unregister call is removed from ~QtDataBase
-            return;
-        }
-
-        Q_ASSERT_X(contains(value), Q_FUNC_INFO, "Can't remove a value which is not in the bag");
-        Q_ASSERT(v->m_prev || (m_first == v && !v->m_prev));
-
-        if (v->m_next)
-            v->m_next->m_prev= v->m_prev;
-
-        if (v->m_prev)
-            v->m_prev->m_next = v->m_next;
-        else
-            m_first = v->m_next;
-        // reset removed value
-        v->m_next = v->m_prev = 0;
-    }
-
-    /*!
-      \internal
-      Call \a fun for each element in this container. Fun should accept T* as a parameter.
-      \note In general it is not allowed to change this container by calling put() or get() unless
-      given value is the same as currently procceded by forEach.
-    */
-    template<class Functor>
-    void forEach(Functor fun)
-    {
-        //dump(Q_FUNC_INFO);
-        QScriptLinkedNode *i = m_first;
-        QScriptLinkedNode *tmp;
-        while (i) {
-            tmp = i;
-            i = i->m_next;
-            fun(static_cast<T*>(tmp));
-        }
-    }
-
-    /*!
-      \internal
-      Clear this container.
-    */
-    void clear()
-    {
-        m_first = 0;
-    }
-
-    /*!
-      \internal
-      Returns true if this container is empty; false otherwise.
-    */
-    bool isEmpty() const
-    {
-        return !m_first;
-    }
-
-//    void dump(const char* msg, T* obj = 0) const
-//    {
-//        qDebug() << msg << obj;
-//        qDebug() << m_first;
-//        QScriptLinkedNode *i = m_first;
-//        while (i) {
-//            qDebug() <<"  - " << i << "(" << i->m_prev << ", " << i->m_next <<")";
-//            i = i->m_next;
-//        }
-//    }
-
-private:
-    bool contains(T *value) const
-    {
-        QScriptLinkedNode *i = m_first;
-        while (i) {
-            if (static_cast<T*>(i) == value)
-                return true;
-            i = i->m_next;
-        }
-        return false;
-    }
-    QScriptLinkedNode *m_first;
-};
+template<class N, QIntrusiveListNode N::*member>
+void QScriptIntrusiveList<N, member>::remove(N *n)
+{
+    Q_ASSERT_X(contains(n), Q_FUNC_INFO, "Can't remove a value which is not in the list");
+    QIntrusiveList<N, member>::remove(n);
+}
 
 QT_END_NAMESPACE
 
diff --git a/src/declarative/qml/v8/qv8engine_impl_p.h b/src/declarative/qml/v8/qv8engine_impl_p.h
index 382e221f5a..53ce2a5acd 100644
--- a/src/declarative/qml/v8/qv8engine_impl_p.h
+++ b/src/declarative/qml/v8/qv8engine_impl_p.h
@@ -99,9 +99,10 @@ inline void QV8Engine::unregisterValue(QJSValuePrivate *data)
 
 inline void QV8Engine::invalidateAllValues()
 {
-    QtScriptBagCleaner invalidator;
-    m_values.forEach(invalidator);
-    m_values.clear();
+    ValueList::iterator it;
+    for (it = m_values.begin(); it != m_values.end(); it = it.erase())
+        (*it)->invalidate();
+    Q_ASSERT(m_values.isEmpty());
 }
 
 inline void QV8Engine::registerValueIterator(QJSValueIteratorPrivate *data)
@@ -116,9 +117,10 @@ inline void QV8Engine::unregisterValueIterator(QJSValueIteratorPrivate *data)
 
 inline void QV8Engine::invalidateAllIterators()
 {
-    QtScriptBagCleaner invalidator;
-    m_valueIterators.forEach(invalidator);
-    m_valueIterators.clear();
+    ValueIteratorList::iterator it;
+    for (it = m_valueIterators.begin(); it != m_valueIterators.end(); it = it.erase())
+        (*it)->invalidate();
+    Q_ASSERT(m_valueIterators.isEmpty());
 }
 
 /*!
diff --git a/src/declarative/qml/v8/qv8engine_p.h b/src/declarative/qml/v8/qv8engine_p.h
index b71e6dc55f..22c3504565 100644
--- a/src/declarative/qml/v8/qv8engine_p.h
+++ b/src/declarative/qml/v8/qv8engine_p.h
@@ -63,6 +63,8 @@
 #include <private/qv8_p.h>
 #include <qjsengine.h>
 #include <qjsvalue.h>
+#include "qjsvalue_p.h"
+#include "qjsvalueiterator_p.h"
 #include "qscriptoriginalglobalobject_p.h"
 #include "qscripttools_p.h"
 
@@ -214,7 +216,6 @@ class QDeclarativeEngine;
 class QDeclarativeValueType;
 class QNetworkAccessManager;
 class QDeclarativeContextData;
-class QJSValueIteratorPrivate;
 
 class Q_DECLARATIVE_EXPORT QV8Engine
 {
@@ -460,8 +461,10 @@ protected:
     double qtDateTimeToJsDate(const QDateTime &dt);
     QDateTime qtDateTimeFromJsDate(double jsDate);
 private:
-    QScriptBagContainer<QJSValuePrivate> m_values;
-    QScriptBagContainer<QJSValueIteratorPrivate> m_valueIterators;
+    typedef QScriptIntrusiveList<QJSValuePrivate, &QJSValuePrivate::m_node> ValueList;
+    ValueList m_values;
+    typedef QScriptIntrusiveList<QJSValueIteratorPrivate, &QJSValueIteratorPrivate::m_node> ValueIteratorList;
+    ValueIteratorList m_valueIterators;
 
     Q_DISABLE_COPY(QV8Engine)
 };