[ovs-dev] [PATCH 1/2] simap: New data structure for string-to-integer maps.

[Ben Pfaff]

On Tue, May 15, 2012 at 05:40:38PM -0700, Ethan Jackson wrote:
> It feels a bit fragile to me that we use hash_name() in some places
> and hash_bytes() in others.  If the implementation of hash_string()
> changes, this is going to break.  Minimally I think we should change
> hash_name() to use hash_bytes() directly instead of going through
> hash_string().  We could also just get rid of simap_find_len() which
> is the only direct caller of hash_bytes(), not sure if it will be used
> later or not.

I decided to make hash_name() take a length and updated the callers.

> s/freee()/free()/

Done.

> The comment of simap_destroy() says it frees 'simap', when it actually
> only frees it's data.

I updated the comment.

> Looks good, thanks  this will be useful.

Here's an incremental.  Thanks for the review.  I'm not going to wait
for a second round since all this is pretty trivial.

diff --git a/lib/simap.c b/lib/simap.c
index 68e58c3..f6804aa 100644
--- a/lib/simap.c
+++ b/lib/simap.c
@@ -19,7 +19,7 @@
 #include <assert.h>
 #include "hash.h"
 
-static size_t hash_name(const char *);
+static size_t hash_name(const char *, size_t length);
 static struct simap_node *simap_find__(const struct simap *,
                                        const char *name, size_t name_len,
                                        size_t hash);
@@ -35,7 +35,7 @@ simap_init(struct simap *simap)
     hmap_init(&simap->map);
 }
 
-/* Frees 'simap' and all the mappings that it contains. */
+/* Frees all the data that 'simap' contains. */
 void
 simap_destroy(struct simap *simap)
 {
@@ -96,15 +96,16 @@ simap_count(const struct simap *simap)
 bool
 simap_put(struct simap *simap, const char *name, unsigned int data)
 {
-    size_t hash = hash_name(name);
+    size_t length = strlen(name);
+    size_t hash = hash_name(name, length);
     struct simap_node *node;
 
-    node = simap_find__(simap, name, strlen(name), hash);
+    node = simap_find__(simap, name, length, hash);
     if (node) {
         node->data = data;
         return false;
     } else {
-        simap_add_nocopy__(simap, xstrdup(name), data, hash);
+        simap_add_nocopy__(simap, xmemdup0(name, length), data, hash);
         return true;
     }
 }
@@ -121,14 +122,16 @@ unsigned int
 simap_increase(struct simap *simap, const char *name, unsigned int amt)
 {
     if (amt) {
-        size_t hash = hash_name(name);
+        size_t length = strlen(name);
+        size_t hash = hash_name(name, length);
         struct simap_node *node;
 
-        node = simap_find__(simap, name, strlen(name), hash);
+        node = simap_find__(simap, name, length, hash);
         if (node) {
             node->data += amt;
         } else {
-            node = simap_add_nocopy__(simap, xstrdup(name), amt, hash);
+            node = simap_add_nocopy__(simap, xmemdup0(name, length),
+                                      amt, hash);
         }
         return node->data;
     } else {
@@ -158,7 +161,7 @@ simap_find(const struct simap *simap, const char *name)
 struct simap_node *
 simap_find_len(const struct simap *simap, const char *name, size_t len)
 {
-    return simap_find__(simap, name, len, hash_bytes(name, len, 0));
+    return simap_find__(simap, name, len, hash_name(name, len));
 }
 
 /* Searches 'simap' for a mapping with the given 'name'.  Returns the
@@ -174,7 +177,7 @@ simap_get(const struct simap *simap, const char *name)
  * ordered alphabetically by name.  The returned array has simap_count(simap)
  * elements.
  *
- * The caller is responsible for freeing the returned array (with freee()).  It
+ * The caller is responsible for freeing the returned array (with free()).  It
  * should not free the individual "simap_node"s in the array, because they are
  * still part of 'simap'. */
 const struct simap_node **
@@ -202,9 +205,9 @@ simap_sort(const struct simap *simap)
 }
 
 static size_t
-hash_name(const char *name)
+hash_name(const char *name, size_t length)
 {
-    return hash_string(name, 0);
+    return hash_bytes(name, length, 0);
 }
 
 static struct simap_node *