Refactor MxHashTable/MxVariableTable (#283)

* Refactor MxHashTable/MxVariableTable

* Use MxS8 for Compare return type

* Cursor::DeleteMatch check and clang fix

LEGO1/mxcollection.h

@@ -0,0 +1,25 @@
+#ifndef MXCOLLECTION_H
+#define MXCOLLECTION_H
+
+#include "mxcore.h"
+
+template <class T>
+class MxCollection : public MxCore {
+public:
+	MxCollection()
+	{
+		m_count = 0;
+		m_customDestructor = Destroy;
+	}
+
+	virtual ~MxCollection() {}
+
+	static void Destroy(T){};
+	virtual MxS8 Compare(T, T) { return 0; }
+
+protected:
+	MxU32 m_count;                 // +0x8
+	void (*m_customDestructor)(T); // +0xc
+};
+
+#endif // MXCOLLECTION_H

LEGO1/mxhashtable.h

@@ -1,13 +1,11 @@
 #ifndef MXHASHTABLE_H
 #define MXHASHTABLE_H
 
+#include "mxcollection.h"
 #include "mxcore.h"
 #include "mxtypes.h"
 
 #define HASH_TABLE_INIT_SIZE 128
-#define HASH_TABLE_OPT_NO_EXPAND 0
-#define HASH_TABLE_OPT_EXPAND_ADD 1
-#define HASH_TABLE_OPT_EXPAND_MULTIPLY 2
 
 template <class T>
 class MxHashTableCursor;
@@ -15,8 +13,7 @@ class MxHashTableCursor;
 template <class T>
 class MxHashTableNode {
 public:
-	MxHashTableNode<T>() {}
-	MxHashTableNode<T>(T* p_obj, MxU32 p_hash)
+	MxHashTableNode<T>(T p_obj, MxU32 p_hash)
 	{
 		m_obj = p_obj;
 		m_hash = p_hash;
@@ -24,54 +21,39 @@ class MxHashTableNode {
 		m_next = NULL;
 	}
 
-	// private:
-	T* m_obj;
+	// DECOMP: Should use getter and setter methods here per the style guide.
+	// However, LEGO1D (with no functions inlined) does not use them.
+	T m_obj;
 	MxU32 m_hash;
 	MxHashTableNode* m_prev;
 	MxHashTableNode* m_next;
 };
 
-// See MxOmni::Create
-// VTABLE 0x100dc1b0
 template <class T>
-class HashTableParent : public MxCore {
+class MxHashTable : protected MxCollection<T> {
 public:
-	HashTableParent()
-	{
-		m_numKeys = 0;
-		m_customDestructor = Destroy;
-	}
+	enum HashTableOpt {
+		HashTableOpt_NoExpand = 0,
+		HashTableOpt_ExpandAdd = 1,
+		HashTableOpt_ExpandMultiply = 2,
+	};
 
-	static void Destroy(T*){};
-
-	virtual MxS8 Compare(T*, T*) = 0;
-
-protected:
-	MxU32 m_numKeys;                // +0x8
-	void (*m_customDestructor)(T*); // +0xc
-};
-
-// VTABLE 0x100dc1e8
-template <class T>
-class MxHashTable : protected HashTableParent<T> {
-public:
 	MxHashTable()
 	{
 		m_numSlots = HASH_TABLE_INIT_SIZE;
 		m_slots = new MxHashTableNode<T>*[HASH_TABLE_INIT_SIZE];
 		memset(m_slots, 0, sizeof(MxHashTableNode<T>*) * m_numSlots);
-		m_resizeOption = HASH_TABLE_OPT_NO_EXPAND;
+		m_resizeOption = HashTableOpt_NoExpand;
 	}
 
 	virtual ~MxHashTable() override;
 
 	void Resize();
-	void Add(T*);
+	void Add(T);
+	void DeleteAll();
 
-	virtual MxS8 Compare(T*, T*) override = 0;
-	virtual MxU32 Hash(T*) = 0;
+	virtual MxU32 Hash(T) { return 0; }
 
-	// FIXME: use of friend here?
 	friend class MxHashTableCursor<T>;
 
 protected:
@@ -79,30 +61,40 @@ class MxHashTable : protected HashTableParent<T> {
 
 	MxHashTableNode<T>** m_slots; // +0x10
 	MxU32 m_numSlots;             // +0x14
-	MxU32 m_autoResizeRatio;
-	int m_resizeOption; // +0x1c
+	MxU32 m_autoResizeRatio;      // +0x18
+	HashTableOpt m_resizeOption;  // +0x1c
 	// FIXME: or FIXME? This qword is used as an integer or double depending
 	// on the value of m_resizeOption. Hard to say whether this is how the devs
 	// did it, but a simple cast in either direction doesn't match.
 	union {
-		MxU32 m_increaseAmount;
-		double m_increaseFactor;
+		MxU32 m_increaseAmount;  // +0x20
+		double m_increaseFactor; // +0x20
 	};
 };
 
 template <class T>
 class MxHashTableCursor : public MxCore {
 public:
-	MxHashTableCursor(MxHashTable<T>* p_hashTable)
+	MxHashTableCursor(MxHashTable<T>* p_table)
 	{
-		m_table = p_hashTable;
+		m_table = p_table;
 		m_match = NULL;
 	}
 
-	MxBool Find(T* p_obj)
-	{
+	MxBool Find(T p_obj);
+	MxBool Current(T& p_obj);
+	void DeleteMatch();
+
+private:
+	MxHashTable<T>* m_table;
+	MxHashTableNode<T>* m_match;
+};
+
+template <class T>
+MxBool MxHashTableCursor<T>::Find(T p_obj)
+{
 	MxU32 hash = m_table->Hash(p_obj);
-		int bucket = hash % m_table->m_numSlots;
+	MxS32 bucket = hash % m_table->m_numSlots;
 
 	MxHashTableNode<T>* t = m_table->m_slots[bucket];
 
@@ -113,26 +105,32 @@ class MxHashTableCursor : public MxCore {
 	}
 
 	return m_match != NULL;
-	}
+}
 
-	void GetMatch(T*& p_obj)
-	{
+template <class T>
+MxBool MxHashTableCursor<T>::Current(T& p_obj)
+{
 	if (m_match) {
 		p_obj = m_match->m_obj;
 	}
-	}
 
-	void DeleteMatch()
-	{
+	return m_match != NULL;
+}
+
+template <class T>
+void MxHashTableCursor<T>::DeleteMatch()
+{
 	// Cut the matching node out of the linked list
 	// by updating pointer references.
+	if (m_match == NULL)
+		return;
 
 	if (m_match->m_prev) {
 		m_match->m_prev->m_next = m_match->m_next;
 	}
 	else {
 		// No "prev" node, so move "next" to the head of the list.
-			int bucket = m_match->m_hash % m_table->m_numSlots;
+		MxS32 bucket = m_match->m_hash % m_table->m_numSlots;
 		m_table->m_slots[bucket] = m_match->m_next;
 	}
 
@@ -141,29 +139,30 @@ class MxHashTableCursor : public MxCore {
 
 	m_table->m_customDestructor(m_match->m_obj);
 	delete m_match;
-		m_table->m_numKeys--;
-	}
-
-private:
-	MxHashTable<T>* m_table;
-	MxHashTableNode<T>* m_match;
-};
+	m_table->m_count--;
+}
 
 template <class T>
 MxHashTable<T>::~MxHashTable()
 {
-	for (int i = 0; i < m_numSlots; i++) {
+	DeleteAll();
+}
+
+template <class T>
+void MxHashTable<T>::DeleteAll()
+{
+	for (MxS32 i = 0; i < m_numSlots; i++) {
 		MxHashTableNode<T>* t = m_slots[i];
 
 		while (t) {
 			MxHashTableNode<T>* next = t->m_next;
-			this->m_customDestructor(t->m_obj);
+			m_customDestructor(t->m_obj);
 			delete t;
 			t = next;
 		}
 	}
 
-	this->m_numKeys = 0;
+	m_count = 0;
 	memset(m_slots, 0, sizeof(MxHashTableNode<T>*) * m_numSlots);
 
 	delete[] m_slots;
@@ -178,21 +177,20 @@ inline void MxHashTable<T>::Resize()
 	MxHashTableNode<T>** old_table = m_slots;
 
 	switch (m_resizeOption) {
-	case HASH_TABLE_OPT_EXPAND_ADD:
+	case HashTableOpt_ExpandAdd:
 		m_numSlots += m_increaseAmount;
 		break;
-	case HASH_TABLE_OPT_EXPAND_MULTIPLY:
+	case HashTableOpt_ExpandMultiply:
 		m_numSlots *= m_increaseFactor;
 		break;
 	}
 
 	MxHashTableNode<T>** new_table = new MxHashTableNode<T>*[m_numSlots];
-	// FIXME: order? m_numKeys set after `rep stosd`
 	m_slots = new_table;
 	memset(m_slots, 0, sizeof(MxHashTableNode<T>*) * m_numSlots);
-	this->m_numKeys = 0;
+	m_count = 0;
 
-	for (int i = 0; i != old_size; i++) {
+	for (MxS32 i = 0; i != old_size; i++) {
 		MxHashTableNode<T>* t = old_table[i];
 
 		while (t) {
@@ -208,7 +206,7 @@ inline void MxHashTable<T>::Resize()
 template <class T>
 inline void MxHashTable<T>::_NodeInsert(MxHashTableNode<T>* p_node)
 {
-	int bucket = p_node->m_hash % m_numSlots;
+	MxS32 bucket = p_node->m_hash % m_numSlots;
 
 	p_node->m_next = m_slots[bucket];
 
@@ -216,13 +214,13 @@ inline void MxHashTable<T>::_NodeInsert(MxHashTableNode<T>* p_node)
 		m_slots[bucket]->m_prev = p_node;
 
 	m_slots[bucket] = p_node;
-	this->m_numKeys++;
+	m_count++;
 }
 
 template <class T>
-inline void MxHashTable<T>::Add(T* p_newobj)
+inline void MxHashTable<T>::Add(T p_newobj)
 {
-	if (m_resizeOption && ((this->m_numKeys + 1) / m_numSlots) > m_autoResizeRatio)
+	if (m_resizeOption && ((m_count + 1) / m_numSlots) > m_autoResizeRatio)
 		MxHashTable<T>::Resize();
 
 	MxU32 hash = Hash(p_newobj);
@@ -231,4 +229,6 @@ inline void MxHashTable<T>::Add(T* p_newobj)
 	MxHashTable<T>::_NodeInsert(node);
 }
 
+#undef HASH_TABLE_INIT_SIZE
+
 #endif // MXHASHTABLE_H

LEGO1/mxstring.h

@@ -18,6 +18,7 @@ class MxString : public MxCore {
 	MxString operator+(const char*);
 	MxString& operator+=(const char*);
 
+	inline MxS8 Compare(const MxString& p_str) const { return strcmp(m_data, p_str.m_data); }
 	inline const char* GetData() const { return m_data; }
 
 private:

LEGO1/mxvariabletable.cpp

@@ -3,7 +3,7 @@
 // OFFSET: LEGO1 0x100b7330
 MxS8 MxVariableTable::Compare(MxVariable* p_var0, MxVariable* p_var1)
 {
-	return strcmp(p_var0->GetKey()->GetData(), p_var1->GetKey()->GetData());
+	return p_var0->GetKey()->Compare(*p_var1->GetKey());
 }
 
 // OFFSET: LEGO1 0x100b7370
@@ -22,43 +22,43 @@ MxU32 MxVariableTable::Hash(MxVariable* p_var)
 // OFFSET: LEGO1 0x100b73a0
 void MxVariableTable::SetVariable(const char* p_key, const char* p_value)
 {
-	MxHashTableCursor<MxVariable> cursor(this);
+	MxHashTableCursor<MxVariable*> cursor(this);
 	MxVariable* var = new MxVariable(p_key, p_value);
 
 	if (cursor.Find(var)) {
 		delete var;
-		cursor.GetMatch(var);
+		cursor.Current(var);
 		var->SetValue(p_value);
 	}
 	else {
-		MxHashTable<MxVariable>::Add(var);
+		MxHashTable<MxVariable*>::Add(var);
 	}
 }
 
 // OFFSET: LEGO1 0x100b7740
-void MxVariableTable::SetVariable(MxVariable* var)
+void MxVariableTable::SetVariable(MxVariable* p_var)
 {
-	MxHashTableCursor<MxVariable> cursor(this);
-	MxBool found = cursor.Find(var);
+	MxHashTableCursor<MxVariable*> cursor(this);
+	MxBool found = cursor.Find(p_var);
 
 	if (found)
 		cursor.DeleteMatch();
 
-	MxHashTable<MxVariable>::Add(var);
+	MxHashTable<MxVariable*>::Add(p_var);
 }
 
 // OFFSET: LEGO1 0x100b78f0
 const char* MxVariableTable::GetVariable(const char* p_key)
 {
 	const char* value = "";
-	MxHashTableCursor<MxVariable> cursor(this);
+	MxHashTableCursor<MxVariable*> cursor(this);
 	MxVariable* var = new MxVariable(p_key);
 
 	MxBool found = cursor.Find(var);
 	delete var;
 
 	if (found) {
-		cursor.GetMatch(var);
+		cursor.Current(var);
 		value = var->GetValue()->GetData();
 	}
 

LEGO1/mxvariabletable.h

@@ -7,12 +7,12 @@
 
 // VTABLE 0x100dc1c8
 // SIZE 0x28
-class MxVariableTable : public MxHashTable<MxVariable> {
+class MxVariableTable : public MxHashTable<MxVariable*> {
 public:
 	MxVariableTable() { m_customDestructor = Destroy; }
-	__declspec(dllexport) void SetVariable(const char* key, const char* value);
-	__declspec(dllexport) void SetVariable(MxVariable* var);
-	__declspec(dllexport) const char* GetVariable(const char* key);
+	__declspec(dllexport) void SetVariable(const char* p_key, const char* p_value);
+	__declspec(dllexport) void SetVariable(MxVariable* p_var);
+	__declspec(dllexport) const char* GetVariable(const char* p_key);
 
 	// OFFSET: LEGO1 0x100afdb0
 	static void Destroy(MxVariable* p_obj) { p_obj->Destroy(); }
@@ -21,13 +21,37 @@ class MxVariableTable : public MxHashTable<MxVariable> {
 	virtual MxU32 Hash(MxVariable*) override;                // +0x18
 };
 
+// OFFSET: LEGO1 0x100afcd0 TEMPLATE
+// MxCollection<MxVariable *>::Compare
+
+// OFFSET: LEGO1 0x100afce0 TEMPLATE
+// MxCollection<MxVariable *>::~MxCollection<MxVariable *>
+
+// OFFSET: LEGO1 0x100afd30 TEMPLATE
+// MxCollection<MxVariable *>::Destroy
+
+// OFFSET: LEGO1 0x100afd40 TEMPLATE
+// MxCollection<MxVariable *>::`scalar deleting destructor'
+
+// OFFSET: LEGO1 0x100afdc0 TEMPLATE
+// MxHashTable<MxVariable *>::Hash
+
 // OFFSET: LEGO1 0x100b0bd0 TEMPLATE
-// MxHashTable<MxVariable>::~MxHashTable<MxVariable>
+// MxHashTable<MxVariable *>::~MxHashTable<MxVariable *>
+
+// OFFSET: LEGO1 0x100b0ca0 TEMPLATE
+// MxHashTable<MxVariable *>::`scalar deleting destructor'
 
 // OFFSET: LEGO1 0x100b7ab0 TEMPLATE
-// MxHashTable<MxVariable>::Resize
+// MxHashTable<MxVariable *>::Resize
 
 // OFFSET: LEGO1 0x100b7b80 TEMPLATE
-// MxHashTable<MxVariable>::_NodeInsert
+// MxHashTable<MxVariable *>::_NodeInsert
+
+// VTABLE 0x100dc1b0 TEMPLATE
+// class MxCollection<MxVariable *>
+
+// VTABLE 0x100dc1e8 TEMPLATE
+// class MxHashTable<MxVariable *>
 
 #endif // MXVARIABLETABLE_H