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 *
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
