Hello Philip,
please read the comments inline. I also attached git diff to demonstrate what I mean to this email.
From: Philip Prindeville philipp@fedoraproject.org
Add c_str as a "const char *" to the hash_key_t.
dhash/dhash.c | 35 ++++++++++++++++++++++++++++++----- dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..5f9f631 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -44,6 +44,7 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#include <stdint.h>
Why did you include stdint?
#include <errno.h> #include "dhash.h"
@@ -70,6 +71,9 @@ } \ } while(0)
+#define discard_const(ptr) ((void *)((intptr_t)(ptr))) +#define discard_const_p(type, ptr) ((type *)discard_const(ptr))
I do not think it is good to use the discard_const macro for this case. See further comments below.
/*****************************************************************************/
/************************** Internal Type Definitions ************************/
/*****************************************************************************/
@@ -176,7 +180,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h;
- unsigned char *k;
const unsigned char *k;
switch(key->type) { case HASH_KEY_ULONG:
@@ -184,7 +188,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */
for (h = 0, k = (unsigned char *) key->str; *k; k++)
for (h = 0, k = (const unsigned char *) key->str; *k; k++)h = h * PRIME_1 ^ (*k - ' ');break;- case HASH_KEY_CONST_STRING:
/* Convert string to integer */for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default:@@ -212,6 +221,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING:
- case HASH_KEY_CONST_STRING: return true; default: return false;
@@ -244,6 +254,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0);
- case HASH_KEY_CONST_STRING:
return (strcmp(a->c_str, b->c_str) == 0); } return false;@@ -671,7 +683,9 @@ int hash_destroy(hash_table_t *table) q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, &p->entry); if (p->entry.key.type == HASH_KEY_STRING) {
hfree(table, (char*)p->entry.key.str);
hfree(table, p->entry.key.str);} else if (p->entry.key.type ==HASH_KEY_CONST_STRING) {
hfree(table,discard_const(p->entry.key.c_str)); } hfree(table, (char *)p); p = q; @@ -978,7 +992,16 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) hfree(table, element); return HASH_ERROR_NO_MEMORY; }
memcpy((void *)element->entry.key.str, key->str, len);
memcpy(element->entry.key.str, key->str, len);break;case HASH_KEY_CONST_STRING:len = strlen(key->c_str)+1;element->entry.key.c_str = halloc(table, len);if (element->entry.key.c_str == NULL) {hfree(table, element);return HASH_ERROR_NO_MEMORY;}memcpy(discard_const(element->entry.key.c_str),key->c_str, len); break;
We do an internal copy here. We do not need to special case the HASH_KEY_CONST_STRING. Internally we can always access the key in the hash table with key.str and the key from the user with key.c_str no matter if the user chose constant or non constant key. We will only modify the internal part during free operation and do not touch the key from user.
}@@ -1071,7 +1094,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) } } if (element->entry.key.type == HASH_KEY_STRING) {
hfree(table, (char *)element->entry.key.str);
hfree(table, element->entry.key.str);} else if (element->entry.key.type == HASH_KEY_CONST_STRING) {hfree(table, discard_const(element->entry.key.c_str)); } hfree(table, element); return HASH_SUCCESS;diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dhash/dhash.h +++ b/dhash/dhash.h @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t;
typedef enum { HASH_KEY_STRING,
- HASH_KEY_ULONG
HASH_KEY_ULONG,
HASH_KEY_CONST_STRING } hash_key_enum;
typedef enum
@@ -137,6 +138,7 @@ typedef struct hash_key_t { hash_key_enum type; union { char *str;
const char *c_str; unsigned long ul; };diff --git a/dhash/examples/dhash_test.c b/dhash/examples/dhash_test.c index 303e9f8..7613e23 100644 --- a/dhash/examples/dhash_test.c +++ b/dhash/examples/dhash_test.c @@ -64,6 +64,9 @@ static char *key_string(hash_key_t *key) case HASH_KEY_STRING: snprintf(buf, sizeof(buf), "key string = "%s"", key->str); break;
- case HASH_KEY_CONST_STRING:
snprintf(buf, sizeof(buf), "key string = \"%s\"", key->c_str);break; default: snprintf(buf, sizeof(buf), "unknown key type = %d",key->type); break;
See the attached git diff with changes that I would like you to do.
Also use command git format-patch -1
and send the new patch in attachment to the mail.
Also the new funtionality needs to be covered in a unit test. I would prefer to create a new file with check based tests rather than expanding the existing dhash tests. Will you write the test or would prefer me to do it (I have no problem doing it)?
Thanks, Michal