Refactor deletion of properties

In line with the previous commit, allow entries with a valid
PropertyKey, but invalid attributes in the InternalClass. Those
entries mark a deleted property.

This cleans up/unifies some of the code in the internal class
implementation and allows re-using the slot if a deleted property
gets added again.

Change-Id: I1bada697486e3cafce7689bae87b7f884200dd99
Reviewed-by: Erik Verbruggen <erik.verbruggen@qt.io>

4 files changed, 46 insertions, 40 deletions

diff --git a/src/qml/jsruntime/qv4global_p.h b/src/qml/jsruntime/qv4global_p.h
index c53465d24e..eab519720f 100644
--- a/src/qml/jsruntime/qv4global_p.h
+++ b/src/qml/jsruntime/qv4global_p.h
@@ -364,6 +364,7 @@ struct PropertyAttributes
     bool isEmpty() const { return !m_all; }
 
     uint flags() const { return m_flags; }
+    uint all() const { return m_all; }
 
     bool operator==(PropertyAttributes other) {
         return m_all == other.m_all;
diff --git a/src/qml/jsruntime/qv4internalclass.cpp b/src/qml/jsruntime/qv4internalclass.cpp
index 2916b09d76..ddb8542e07 100644
--- a/src/qml/jsruntime/qv4internalclass.cpp
+++ b/src/qml/jsruntime/qv4internalclass.cpp
@@ -309,10 +309,12 @@ static void addDummyEntry(InternalClass *newClass, PropertyHash::Entry e)
 
 Heap::InternalClass *InternalClass::changeMember(PropertyKey identifier, PropertyAttributes data, InternalClassEntry *entry)
 {
-    data.resolve();
+    if (!data.isEmpty())
+        data.resolve();
     PropertyHash::Entry *e = findEntry(identifier);
     Q_ASSERT(e && e->index != UINT_MAX);
     uint idx = e->index;
+    Q_ASSERT(idx != UINT_MAX);
 
     if (entry) {
         entry->index = idx;
@@ -323,7 +325,7 @@ Heap::InternalClass *InternalClass::changeMember(PropertyKey identifier, Propert
     if (data == propertyData.at(idx))
         return static_cast<Heap::InternalClass *>(this);
 
-    Transition temp = { { identifier }, nullptr, (int)data.flags() };
+    Transition temp = { { identifier }, nullptr, int(data.all()) };
     Transition &t = lookupOrInsertTransition(temp);
     if (t.lookup)
         return t.lookup;
@@ -413,7 +415,8 @@ Heap::InternalClass *InternalClass::nonExtensible()
 void InternalClass::addMember(QV4::Object *object, PropertyKey id, PropertyAttributes data, InternalClassEntry *entry)
 {
     Q_ASSERT(id.isStringOrSymbol());
-    data.resolve();
+    if (!data.isEmpty())
+        data.resolve();
     PropertyHash::Entry *e = object->internalClass()->findEntry(id);
     if (e) {
         changeMember(object, id, data, entry);
@@ -427,7 +430,8 @@ void InternalClass::addMember(QV4::Object *object, PropertyKey id, PropertyAttri
 Heap::InternalClass *InternalClass::addMember(PropertyKey identifier, PropertyAttributes data, InternalClassEntry *entry)
 {
     Q_ASSERT(identifier.isStringOrSymbol());
-    data.resolve();
+    if (!data.isEmpty())
+        data.resolve();
 
     PropertyHash::Entry *e = findEntry(identifier);
     if (e)
@@ -483,26 +487,17 @@ void InternalClass::removeChildEntry(InternalClass *child)
 
 void InternalClass::removeMember(QV4::Object *object, PropertyKey identifier)
 {
+#ifndef QT_NO_DEBUG
     Heap::InternalClass *oldClass = object->internalClass();
-    Q_ASSERT(oldClass->propertyTable.lookup(identifier) && oldClass->propertyTable.lookup(identifier)->index < oldClass->size);
-
-    Transition temp = { { identifier }, nullptr, Transition::RemoveMember };
-    Transition &t = object->internalClass()->lookupOrInsertTransition(temp);
-
-    if (!t.lookup) {
-        // create a new class and add it to the tree
-        Heap::InternalClass *newClass = oldClass->engine->newClass(oldClass);
-        // simply make the entry inaccessible
-        int idx = newClass->propertyTable.removeIdentifier(identifier, oldClass->size);
-        newClass->nameMap.set(idx, PropertyKey::invalid());
-        newClass->propertyData.set(idx, PropertyAttributes());
-        t.lookup = newClass;
-        Q_ASSERT(t.lookup);
-    }
-    object->setInternalClass(t.lookup);
+    Q_ASSERT(oldClass->findEntry(identifier) != nullptr);
+#endif
+
+    changeMember(object, identifier, Attr_Invalid);
 
+#ifndef QT_NO_DEBUG
     // we didn't remove the data slot, just made it inaccessible
     Q_ASSERT(object->internalClass()->size == oldClass->size);
+#endif
 }
 
 Heap::InternalClass *InternalClass::sealed()
@@ -610,10 +605,12 @@ Heap::InternalClass *InternalClass::propertiesFrozen() const
     frozen = frozen->changePrototype(prototype);
     for (uint i = 0; i < size; ++i) {
         PropertyAttributes attrs = propertyData.at(i);
-        if (attrs.isEmpty())
+        if (!nameMap.at(i).isValid())
             continue;
-        attrs.setWritable(false);
-        attrs.setConfigurable(false);
+        if (!attrs.isEmpty()) {
+            attrs.setWritable(false);
+            attrs.setConfigurable(false);
+        }
         frozen = frozen->addMember(nameMap.at(i), attrs);
     }
     return frozen->d();
diff --git a/src/qml/jsruntime/qv4internalclass_p.h b/src/qml/jsruntime/qv4internalclass_p.h
index 2d60a0048f..681cbda5f9 100644
--- a/src/qml/jsruntime/qv4internalclass_p.h
+++ b/src/qml/jsruntime/qv4internalclass_p.h
@@ -314,8 +314,7 @@ struct InternalClassTransition
         PrototypeChange = 0x201,
         ProtoClass = 0x202,
         Sealed = 0x203,
-        Frozen = 0x204,
-        RemoveMember = -1
+        Frozen = 0x204
     };
 
     bool operator==(const InternalClassTransition &other) const
@@ -378,7 +377,8 @@ struct InternalClass : Base {
         PropertyHash::Entry *e = propertyTable.lookup(id);
         if (e && e->index < size) {
             PropertyAttributes a = propertyData.at(e->index);
-            return { e->index, e->setterIndex, a };
+            if (!a.isEmpty())
+                return { e->index, e->setterIndex, a };
         }
 
         return { UINT_MAX, UINT_MAX, Attr_Invalid };
@@ -395,8 +395,11 @@ struct InternalClass : Base {
         Q_ASSERT(id.isStringOrSymbol());
 
         PropertyHash::Entry *e = propertyTable.lookup(id);
-        if (e && e->index < size)
-            return { e->index, propertyData.at(e->index) };
+        if (e && e->index < size) {
+            PropertyAttributes a = propertyData.at(e->index);
+            if (!a.isEmpty())
+                return { e->index, a };
+        }
 
         return { UINT_MAX, Attr_Invalid };
     }
@@ -408,11 +411,13 @@ struct InternalClass : Base {
         PropertyHash::Entry *e = propertyTable.lookup(id);
         if (e && e->index < size) {
             PropertyAttributes a = propertyData.at(e->index);
-            if (a.isAccessor()) {
-                Q_ASSERT(e->setterIndex != UINT_MAX);
-                return { e->setterIndex, a };
+            if (!a.isEmpty()) {
+                if (a.isAccessor()) {
+                    Q_ASSERT(e->setterIndex != UINT_MAX);
+                    return { e->setterIndex, a };
+                }
+                return { e->index, a };
             }
-            return { e->index, a };
         }
 
         return { UINT_MAX, Attr_Invalid };
@@ -423,8 +428,10 @@ struct InternalClass : Base {
         Q_ASSERT(id.isStringOrSymbol());
 
         PropertyHash::Entry *e = propertyTable.lookup(id);
-        if (e && e->index < size)
+        if (e && e->index < size) {
+            Q_ASSERT(!propertyData.at(e->index).isEmpty());
             return e->index;
+        }
 
         return UINT_MAX;
     }
@@ -434,8 +441,10 @@ struct InternalClass : Base {
         Q_ASSERT(id.isStringOrSymbol());
 
         PropertyHash::Entry *e = propertyTable.lookup(id);
-        if (e && e->index < size)
+        if (e && e->index < size) {
+            Q_ASSERT(!propertyData.at(e->index).isEmpty());
             return e->index == index;
+        }
 
         return false;
     }
diff --git a/src/qml/jsruntime/qv4object.cpp b/src/qml/jsruntime/qv4object.cpp
index b6446d39d1..ea374e43c0 100644
--- a/src/qml/jsruntime/qv4object.cpp
+++ b/src/qml/jsruntime/qv4object.cpp
@@ -364,18 +364,17 @@ PropertyKey ObjectOwnPropertyKeyIterator::next(const Object *o, Property *pd, Pr
         while (memberIndex < o->internalClass()->size) {
             PropertyKey n = o->internalClass()->nameMap.at(memberIndex);
             ++memberIndex;
-            if (!n.isStringOrSymbol()) {
+            if (!n.isStringOrSymbol())
                 // accessor properties have a dummy entry with n == 0
                 continue;
-            }
-            if (!iterateOverSymbols && n.isSymbol()) {
+            if (!iterateOverSymbols && n.isSymbol())
                 continue;
-            }
-            if (iterateOverSymbols && !n.isSymbol()) {
+            if (iterateOverSymbols && !n.isSymbol())
                 continue;
-            }
 
             InternalClassEntry e = o->internalClass()->find(n);
+            if (!e.isValid())
+                continue;
             if (pd) {
                 pd->value = *o->propertyData(e.index);
                 if (e.attributes.isAccessor())