Sounds good thanks.
Ethan
On Tue, May 22, 2012 at 10:32 AM, Ben Pfaff <b...@nicira.com> wrote:
> 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
