From 31e4a8432bc227b0f00039c713b0a28602b6167e Mon Sep 17 00:00:00 2001
From: Enno Rehling <enno.rehling@gmail.com>
Date: Tue, 22 May 2012 18:44:54 -0700
Subject: [PATCH] fix find_spell and create_spell to only create each spell
 once.

kill register_spell
make some critbit changes (need to transfer them upstream)
---
 src/bindings/bind_region.c |  5 ++-
 src/kernel/config.c        |  7 ++--
 src/kernel/item.c          | 12 +++---
 src/kernel/spell.c         | 61 +++++++++++++++++-----------
 src/kernel/spell.h         | 23 +++--------
 src/kernel/spell_test.c    | 42 +++++++++++++++----
 src/kernel/xmlreader.c     | 83 +++++++++++++++++---------------------
 src/tests.c                |  1 +
 src/util/critbit.c         | 12 +++---
 src/util/critbit.h         |  2 +-
 src/util/functions.c       |  4 +-
 src/util/quicklist.c       | 17 ++++----
 src/util/translation.c     |  4 +-
 13 files changed, 152 insertions(+), 121 deletions(-)

diff --git a/src/bindings/bind_region.c b/src/bindings/bind_region.c
index c98849dfa..8404b2d92 100644
--- a/src/bindings/bind_region.c
+++ b/src/bindings/bind_region.c
@@ -319,8 +319,9 @@ static critbit_tree * special_resources(void)
     char buffer[32];
     int i;
     for (i=0;special[i];++i) {
-      cb_new_kv(special[i], &i, sizeof(int), buffer);
-      cb_insert(&cb, buffer, strlen(special[i])+1+sizeof(int));
+      size_t len = strlen(special[i]);
+      len = cb_new_kv(special[i], len, &i, sizeof(int), buffer);
+      cb_insert(&cb, buffer, len);
     }
   }
   return &cb;
diff --git a/src/kernel/config.c b/src/kernel/config.c
index 6d1d753fd..7033e4ba8 100644
--- a/src/kernel/config.c
+++ b/src/kernel/config.c
@@ -1989,11 +1989,12 @@ static void init_translations(const struct locale *lang, int ut, const char * (*
       char * str = transliterate(buffer, sizeof(buffer)-sizeof(int), key);
       if (str) {
         critbit_tree * cb = (critbit_tree *)*tokens;
-        if (!cb) {
+		size_t len = strlen(str);
+		if (!cb) {
           *tokens = cb = (critbit_tree *)calloc(1, sizeof(critbit_tree *));
         }
-        cb_new_kv(str, &i, sizeof(int), buffer);
-        cb_insert(cb, buffer, strlen(str)+1+sizeof(int));
+        len = cb_new_kv(str, len, &i, sizeof(int), buffer);
+        cb_insert(cb, buffer, len);
       } else {
         log_error("could not transliterate '%s'\n", key);
       }
diff --git a/src/kernel/item.c b/src/kernel/item.c
index 5e842b26b..a432b6226 100644
--- a/src/kernel/item.c
+++ b/src/kernel/item.c
@@ -177,8 +177,8 @@ void it_register(item_type * itype)
   size_t len = strlen(name);
 
   assert(len<sizeof(buffer)-sizeof(itype));
-  cb_new_kv(name, &itype, sizeof(itype), buffer);
-  if (cb_insert(&cb_items, buffer, len+1+sizeof(itype))) {
+  len = cb_new_kv(name, len, &itype, sizeof(itype), buffer);
+  if (cb_insert(&cb_items, buffer, len)) {
     rt_register(itype->rtype);
   }
 }
@@ -298,8 +298,8 @@ void rt_register(resource_type * rtype)
   size_t len = strlen(name);
 
   assert(len<sizeof(buffer)-sizeof(rtype));
-  cb_new_kv(name, &rtype, sizeof(rtype), buffer);
-  cb_insert(&cb_resources, buffer, len+1+sizeof(rtype));
+  len = cb_new_kv(name, len, &rtype, sizeof(rtype), buffer);
+  cb_insert(&cb_resources, buffer, len);
 }
 
 const resource_type *item2resource(const item_type * itype)
@@ -1116,8 +1116,8 @@ static int add_itemname_cb(const void * match, const void * key, size_t keylen,
     if (name && transliterate(buffer, sizeof(buffer), name)) {
       size_t len = strlen(buffer);
       assert(len+sizeof(itype)<sizeof(buffer));
-      cb_new_kv(buffer, &itype, sizeof(itype), buffer);
-      cb_insert(cb, buffer, len+1+sizeof(itype));
+      len = cb_new_kv(buffer, len, &itype, sizeof(itype), buffer);
+      cb_insert(cb, buffer, len);
     }
   }
   return 0;
diff --git a/src/kernel/spell.c b/src/kernel/spell.c
index ea3e41cef..36bdcaa10 100644
--- a/src/kernel/spell.c
+++ b/src/kernel/spell.c
@@ -24,47 +24,62 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 #include "magic.h"
 #include "unit.h"
 
-/* libc includes */
-#include <assert.h>
-#include <string.h>
-
 /* util includes */
+#include <util/critbit.h>
 #include <util/goodies.h>
 #include <util/language.h>
 #include <util/log.h>
 #include <util/umlaut.h>
 #include <util/quicklist.h>
 
-quicklist *spells = NULL;
+/* libc includes */
+#include <assert.h>
+#include <string.h>
 
-spell * create_spell(const char * name)
-{
-  spell * sp = (spell *) calloc(1, sizeof(spell));
-  sp->sname = strdup(name);
-  return sp;
+static critbit_tree cb_spells;
+quicklist * spells;
+
+void free_spells(void) {
+  cb_clear(&cb_spells);
+  ql_free(spells);
+  spells = 0;
 }
 
-void register_spell(spell * sp)
+spell * create_spell(const char * name, unsigned int id)
 {
-  if (sp->id == 0) {
-    sp->id = hashstring(sp->sname);
+  spell * sp;
+  char buffer[64];
+  size_t len = strlen(name);
+
+  assert(len+sizeof(sp)<sizeof(buffer));
+  if (find_spell(name)) {
+    log_error("create_spell: duplicate name '%s'\n", name);
+    return 0;
   }
-  add_spell(&spells, sp);
+  sp = (spell *) calloc(1, sizeof(spell));
+  len = cb_new_kv(name, len, &sp, sizeof(sp), buffer);
+  if (cb_insert(&cb_spells, buffer, len)) {
+    sp->id = id ? id : hashstring(name);
+    sp->sname = strdup(name);
+    add_spell(&spells, sp);
+    return sp;
+  }
+  free(sp);
+  return 0;
 }
 
 spell *find_spell(const char *name)
 {
-  quicklist *ql = spells;
-  int qi;
+  const char * match;
+  spell * sp = 0;
 
-  for (qi = 0; ql; ql_advance(&ql, &qi, 1)) {
-    spell *sp = (spell *) ql_get(ql, qi);
-    if (strcmp(name, sp->sname) == 0) {
-      return sp;
-    }
+  match = cb_find_str(&cb_spells, name);
+  if (match) {
+    cb_get_kv(match, &sp, sizeof(sp));
+  } else {
+    log_warning("find_spell: could not find spell '%s'\n", name);
   }
-  log_warning("find_spell: could not find spell '%s'\n", name);
-  return 0;
+  return sp;
 }
 
 /* ------------------------------------------------------------- */
diff --git a/src/kernel/spell.h b/src/kernel/spell.h
index 4e5daf1b5..226de96e4 100644
--- a/src/kernel/spell.h
+++ b/src/kernel/spell.h
@@ -22,16 +22,6 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 extern "C" {
 #endif
 
-  struct fighter;
-  struct spell;
-  struct border_type;
-  struct attrib_type;
-  struct curse_type;
-  struct castorder;
-  struct curse;
-  struct region;
-  struct unit;
-
   /* Prototypen */
 
   int use_item_power(struct region *r, struct unit *u);
@@ -44,15 +34,14 @@ extern "C" {
   extern struct attrib_type at_unitdissolve;
   extern struct attrib_type at_wdwpyramid;
 
-  extern struct quicklist *spells;
-
-  extern struct spell * create_spell(const char * name);
-  extern void register_spell(struct spell *sp);
-  extern struct spell *find_spell(const char *name);
-  extern struct spell *find_spellbyid(unsigned int i);
-  extern struct spell *get_spellfromtoken(struct sc_mage *mage, const char *s,
+  extern struct spell * create_spell(const char * name, unsigned int id);
+  extern struct spell * find_spell(const char *name);
+  extern struct spell * find_spellbyid(unsigned int i);
+  extern struct spell * get_spellfromtoken(struct sc_mage *mage, const char *s,
     const struct locale *lang);
+  extern void free_spells(void);
 
+  extern struct quicklist * spells;
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/kernel/spell_test.c b/src/kernel/spell_test.c
index ebcb74dfe..dc8fc8d47 100644
--- a/src/kernel/spell_test.c
+++ b/src/kernel/spell_test.c
@@ -1,4 +1,7 @@
+#include <platform.h>
+
 #include <util/quicklist.h>
+#include <kernel/types.h>
 #include <kernel/spell.h>
 #include <kernel/magic.h>
 
@@ -7,23 +10,48 @@
 
 #include <stdlib.h>
 
-static void test_register_spell(CuTest * tc)
+static void test_create_spell(CuTest * tc)
 {
   spell * sp;
 
+  test_cleanup();
+  CuAssertPtrEquals(tc, 0, spells);
   CuAssertPtrEquals(tc, 0, find_spell("testspell"));
 
-  CuAssertPtrEquals(tc, spells, 0);
-  sp = create_spell("testspell");
-  sp->magietyp = 5;
-  register_spell(sp);
-  CuAssertIntEquals(tc, 1, ql_length(spells));
+  sp = create_spell("testspell", 0);
   CuAssertPtrEquals(tc, sp, find_spell("testspell"));
+  CuAssertPtrNotNull(tc, spells);
+}
+
+static void test_create_duplicate_spell(CuTest * tc)
+{
+  spell *sp;
+
+  test_cleanup();
+  CuAssertPtrEquals(tc, 0, find_spell("testspell"));
+
+  sp = create_spell("testspell", 0);
+  CuAssertPtrEquals(tc, 0, create_spell("testspell", 0));
+  CuAssertPtrEquals(tc, sp, find_spell("testspell"));
+}
+
+static void test_create_spell_with_id(CuTest * tc)
+{
+  spell *sp;
+
+  test_cleanup();
+  CuAssertPtrEquals(tc, 0, find_spellbyid(42));
+  sp = create_spell("testspell", 42);
+  CuAssertPtrEquals(tc, sp, find_spellbyid(42));
+  CuAssertPtrEquals(tc, 0, create_spell("testspell", 47));
+  CuAssertPtrEquals(tc, 0, find_spellbyid(47));
 }
 
 CuSuite *get_spell_suite(void)
 {
   CuSuite *suite = CuSuiteNew();
-  SUITE_ADD_TEST(suite, test_register_spell);
+  SUITE_ADD_TEST(suite, test_create_spell);
+  SUITE_ADD_TEST(suite, test_create_duplicate_spell);
+  SUITE_ADD_TEST(suite, test_create_spell_with_id);
   return suite;
 }
diff --git a/src/kernel/xmlreader.c b/src/kernel/xmlreader.c
index e57176d6c..123c1cf04 100644
--- a/src/kernel/xmlreader.c
+++ b/src/kernel/xmlreader.c
@@ -1481,13 +1481,14 @@ static int parse_spells(xmlDocPtr doc)
       int k;
       spell_component *component;
       spell *sp;
-      int valid = 1;
+      unsigned int index;
       static int modes[] = { 0, PRECOMBATSPELL, COMBATSPELL, POSTCOMBATSPELL };
 
       /* spellname */
+      index = xml_ivalue(node, "index", 0);
       propValue = xmlGetProp(node, BAD_CAST "name");
       assert(propValue != NULL);
-      sp = create_spell((const char *)propValue);
+      sp = create_spell((const char *)propValue, index);
       xmlFree(propValue);
 
       propValue = xmlGetProp(node, BAD_CAST "parameters");
@@ -1513,7 +1514,6 @@ static int parse_spells(xmlDocPtr doc)
       xmlFree(propValue);
 
       /* level, rank and flags */
-      sp->id = xml_ivalue(node, "index", 0);
       sp->level = xml_ivalue(node, "level", -1);
       sp->rank = (char)xml_ivalue(node, "rank", -1);
       if (xml_bvalue(node, "los", false))
@@ -1544,7 +1544,6 @@ static int parse_spells(xmlDocPtr doc)
           cast = get_function(sp->sname);
           if (!cast) {
             log_error("no spell cast function registered for '%s'\n", sp->sname);
-            valid = 0;
           }
           strlcpy(zText+7, sp->sname, sizeof(zText)-7);
           fumble = get_function(zText);
@@ -1558,10 +1557,8 @@ static int parse_spells(xmlDocPtr doc)
             if (strcmp((const char *)propValue, "cast") == 0) {
               if (fun) {
                 cast = fun;
-                valid = 1;
               } else {
                 log_error("unknown function name '%s' for spell '%s'\n", (const char *)propValue, sp->sname);
-                valid = 0;
               }
             } else if (fun && strcmp((const char *)propValue, "fumble") == 0) {
               fumble = fun;
@@ -1576,49 +1573,43 @@ static int parse_spells(xmlDocPtr doc)
         xmlXPathFreeObject(result);
       }
 
-      if (valid) {
-        /* reading eressea/spells/spell/resource */
-        xpath->node = node;
-        result = xmlXPathEvalExpression(BAD_CAST "resource", xpath);
-        if (result->nodesetval->nodeNr) {
-          sp->components =
-            (spell_component *) malloc(sizeof(spell_component) *
-            (result->nodesetval->nodeNr + 1));
-          sp->components[result->nodesetval->nodeNr].type = 0;
-        }
-        for (component = sp->components, k = 0; k != result->nodesetval->nodeNr;
-          ++k) {
-          const resource_type *rtype;
-          xmlNodePtr node = result->nodesetval->nodeTab[k];
-          propValue = xmlGetProp(node, BAD_CAST "name");
-          assert(propValue);
-          rtype = rt_find((const char *)propValue);
-          if (!rtype) {
-            log_error("spell %s uses unknown component %s.\n", sp->sname, (const char *)propValue);
-            xmlFree(propValue);
-            continue;
-          }
-          component->type = rtype;
+      /* reading eressea/spells/spell/resource */
+      xpath->node = node;
+      result = xmlXPathEvalExpression(BAD_CAST "resource", xpath);
+      if (result->nodesetval->nodeNr) {
+        sp->components =
+          (spell_component *) malloc(sizeof(spell_component) *
+          (result->nodesetval->nodeNr + 1));
+        sp->components[result->nodesetval->nodeNr].type = 0;
+      }
+      for (component = sp->components, k = 0; k != result->nodesetval->nodeNr;
+        ++k) {
+        const resource_type *rtype;
+        xmlNodePtr node = result->nodesetval->nodeTab[k];
+        propValue = xmlGetProp(node, BAD_CAST "name");
+        assert(propValue);
+        rtype = rt_find((const char *)propValue);
+        if (!rtype) {
+          log_error("spell %s uses unknown component %s.\n", sp->sname, (const char *)propValue);
           xmlFree(propValue);
-          component->amount = xml_ivalue(node, "amount", 1);
-          component->cost = SPC_FIX;
-          propValue = xmlGetProp(node, BAD_CAST "cost");
-          if (propValue != NULL) {
-            if (strcmp((const char *)propValue, "linear") == 0) {
-              component->cost = SPC_LINEAR;
-            } else if (strcmp((const char *)propValue, "level") == 0) {
-              component->cost = SPC_LEVEL;
-            }
-            xmlFree(propValue);
-          }
-          component++;
+          continue;
         }
-        xmlXPathFreeObject(result);
-      }
-
-      if (valid) {
-        register_spell(sp);
+        component->type = rtype;
+        xmlFree(propValue);
+        component->amount = xml_ivalue(node, "amount", 1);
+        component->cost = SPC_FIX;
+        propValue = xmlGetProp(node, BAD_CAST "cost");
+        if (propValue != NULL) {
+          if (strcmp((const char *)propValue, "linear") == 0) {
+            component->cost = SPC_LINEAR;
+          } else if (strcmp((const char *)propValue, "level") == 0) {
+            component->cost = SPC_LEVEL;
+          }
+          xmlFree(propValue);
+        }
+        component++;
       }
+      xmlXPathFreeObject(result);
     }
   }
 
diff --git a/src/tests.c b/src/tests.c
index ab345bd88..46e5c4304 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -109,6 +109,7 @@ void test_cleanup(void)
   global.functions.wage = NULL;
   default_locale = 0;
   locales = 0; /* TODO: this is evil and leaky */
+  free_spells(); /* TODO: this is just as bad! */
   free_gamedata();
 }
 
diff --git a/src/util/critbit.c b/src/util/critbit.c
index bfedfed7b..7474ec64b 100644
--- a/src/util/critbit.c
+++ b/src/util/critbit.c
@@ -46,6 +46,7 @@ void * make_external_node(const void * key, size_t keylen)
   ptrdiff_t numvalue = (char *)data - (char*)0;
   assert((numvalue&1)==0);
 #endif
+  assert(keylen);
   memcpy(data, &keylen, sizeof(size_t));
   memcpy(data+sizeof(size_t), key, keylen);
   return (void*)(data+1);
@@ -286,7 +287,7 @@ const void * cb_find(critbit_tree * cb, const void * key, size_t keylen)
     ptr = node->child[branch];
   }
   from_external_node(ptr, &str, &len);
-  if (len==keylen && memcmp(key, str, keylen)==0) {
+  if (len>=keylen && memcmp(key, str, keylen)==0) {
     return str;
   }
   return 0;
@@ -330,14 +331,15 @@ int cb_erase(critbit_tree * cb, const void * key, size_t keylen)
   }
 }
 
-size_t cb_new_kv(const char *key, void * value, size_t len, void * dst)
+size_t cb_new_kv(const char *key, size_t keylen, void * value, size_t len, void * out)
 {
-  size_t keylen = strlen(key)+1;
+  char * dst = (char*)out;
   if (dst!=key) {
     memcpy(dst, key, keylen);
   }
-  memcpy((char*)dst+keylen, value, len);
-  return len+keylen;
+  dst[keylen] = 0;
+  memcpy(dst+keylen+1, value, len);
+  return len+keylen+1;
 }
 
 void cb_get_kv(const void *kv, void * value, size_t len)
diff --git a/src/util/critbit.h b/src/util/critbit.h
index 67534d83f..9ce95599b 100644
--- a/src/util/critbit.h
+++ b/src/util/critbit.h
@@ -47,7 +47,7 @@ void cb_clear(critbit_tree * cb);
 #define cb_find_prefix_str(cb, key, results, numresults, offset) \
   cb_find_prefix(cb, (void *)key, strlen(key), results, numresults, offset)
 
-size_t cb_new_kv(const char *key, void * value, size_t len, void * dst);
+size_t cb_new_kv(const char *key, size_t keylen, void * value, size_t len, void * out);
 void cb_get_kv(const void *kv, void * value, size_t len);
 
 #ifdef __cplusplus
diff --git a/src/util/functions.c b/src/util/functions.c
index 7e22e7293..a1605cea4 100644
--- a/src/util/functions.c
+++ b/src/util/functions.c
@@ -45,6 +45,6 @@ void register_function(pf_generic fun, const char *name)
   size_t len = strlen(name);
 
   assert(len<sizeof(buffer)-sizeof(fun));
-  cb_new_kv(name, &fun, sizeof(fun), buffer);
-  cb_insert(&cb_functions, buffer, len+1+sizeof(fun));
+  len = cb_new_kv(name, len, &fun, sizeof(fun), buffer);
+  cb_insert(&cb_functions, buffer, len);
 }
diff --git a/src/util/quicklist.c b/src/util/quicklist.c
index 81bad276b..0e8842ccb 100644
--- a/src/util/quicklist.c
+++ b/src/util/quicklist.c
@@ -19,9 +19,12 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 #include <string.h>
 #include <errno.h>
 
-#define QL_MAXSIZE 14           /* total struct is 64 bytes */
-#define QL_LIMIT 7
+#define QL_MAXSIZE 14 /* max. number of elements unrolled into one node */
+#define QL_LIMIT 7 /* this many or fewer number in a node => attempt merge */
 
+/* The total size of this struct is 64 bytes on a 32-bit system with
+ * normal alignment. YMMV, so on a 64-bit system, twiddle the
+ * constants above */
 struct quicklist {
   struct quicklist *next;
   int num_elements;
@@ -170,11 +173,11 @@ int ql_advance(struct quicklist **iterator, int *index, int stride)
 
 void ql_free(struct quicklist *ql)
 {
-  if (!ql)
-    return;
-  if (ql->next)
-    ql_free(ql->next);
-  free(ql);
+  while (ql) {
+    quicklist * qn = ql;
+    ql = ql->next;
+    free(qn);
+  }
 }
 
 int ql_set_remove(struct quicklist **qlp, void *data)
diff --git a/src/util/translation.c b/src/util/translation.c
index 365b463c1..411b8aee0 100644
--- a/src/util/translation.c
+++ b/src/util/translation.c
@@ -162,8 +162,8 @@ void add_function(const char *symbol, evalfun parse)
   size_t len = strlen(symbol);
   
   assert(len+1+sizeof(parse)<=sizeof(buffer));
-  cb_new_kv(symbol, &parse, sizeof(parse), buffer);
-  cb_insert(&functions, buffer, len+1+sizeof(parse));
+  len = cb_new_kv(symbol, len, &parse, sizeof(parse), buffer);
+  cb_insert(&functions, buffer, len);
 }
 
 static evalfun find_function(const char *symbol)