Re: [PATCH olang 2/2] utils: create hash map data structure

["Carlos Maniero" <carlos@maniero.me>]

Nice work man! I've just a few suggestions.

Changes I'm proposing:
- replace __strdup with a arena function.
- create a function to calculate the hash index.
- some style changes on map_put

---->8----
diff --git a/src/arena.c b/src/arena.c
index ae33e6a..d420002 100644
--- a/src/arena.c
+++ b/src/arena.c
@@ -17,6 +17,7 @@
 #include "arena.h"
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 
 arena_t
 arena_new(size_t size)
@@ -51,3 +52,16 @@ arena_free(arena_t *arena)
     arena->size = 0;
     free(arena->region);
 }
+
+char *
+arena_strdup(arena_t *arena, const char *s)
+{
+    size_t slen = strlen(s);
+    char *result = arena_alloc(arena, slen + 1);
+    if (result == NULL) {
+        return NULL;
+    }
+
+    memcpy(result, s, slen + 1);
+    return result;
+}
diff --git a/src/arena.h b/src/arena.h
index 157165c..230eb5b 100644
--- a/src/arena.h
+++ b/src/arena.h
@@ -38,4 +38,7 @@ arena_release(arena_t *arena);
 void
 arena_free(arena_t *arena);
 
+char *
+arena_strdup(arena_t *arena, const char *s);
+
 #endif
diff --git a/src/map.c b/src/map.c
index 532ba3b..849130e 100644
--- a/src/map.c
+++ b/src/map.c
@@ -31,8 +31,8 @@ u32_fnv1a_hash(const char *s);
 static void
 map_init(map_t *map);
 
-static char *
-__strdup(const char *s, arena_t *arena);
+static uint32_t
+map_index(map_t *map, uint32_t hash);
 
 map_t *
 map_new(arena_t *arena)
@@ -78,24 +78,27 @@ map_put(map_t *map, char *key, void *value)
 {
     assert(map && key);
     uint32_t hash = u32_fnv1a_hash(key);
-    map_entry_t *entry = &(map->entries[hash & (map->capacity - 1)]);
+    map_entry_t *entry = map->entries + map_index(map, hash);
 
-    while (entry != NULL) {
+    if (entry == NULL) {
+        *entry = (map_entry_t){ .key = arena_strdup(map->arena, key), .hash = hash, .value = value, .next = NULL };
+        return true;
+    }
+
+    do {
         if (entry->hash == hash && strcmp(entry->key, key) == 0) {
             entry->value = value;
-            return true;
+            break;
         }
-        if (entry->next == NULL)
+        if (entry->next == NULL) {
+            entry->next = (map_entry_t *)arena_alloc(map->arena, sizeof(map_entry_t));
+            *entry->next =
+                (map_entry_t){ .key = arena_strdup(map->arena, key), .hash = hash, .value = value, .next = NULL };
+
             break;
+        }
         entry = entry->next;
-    }
-
-    if (entry->key == NULL) {
-        *entry = (map_entry_t){ .key = __strdup(key, map->arena), .hash = hash, .value = value, .next = NULL };
-    } else {
-        entry->next = (map_entry_t *)arena_alloc(map->arena, sizeof(map_entry_t));
-        *entry->next = (map_entry_t){ .key = __strdup(key, map->arena), .hash = hash, .value = value, .next = NULL };
-    }
+    } while (entry != NULL);
 
     return true;
 }
@@ -104,7 +107,7 @@ void *
 map_get(map_t *map, char *key)
 {
     uint32_t hash = u32_fnv1a_hash(key);
-    map_entry_t *entry = &map->entries[hash & (map->capacity - 1)];
+    map_entry_t *entry = map->entries + map_index(map, hash);
     while (entry != NULL) {
         if (entry->hash == hash && strcmp(entry->key, key) == 0) {
             return entry->value;
@@ -114,15 +117,9 @@ map_get(map_t *map, char *key)
     return NULL;
 }
 
-static char *
-__strdup(const char *s, arena_t *arena)
+static uint32_t
+map_index(map_t *map, uint32_t hash)
 {
-    size_t slen = strlen(s);
-    char *result = arena_alloc(arena, slen + 1);
-    if (result == NULL) {
-        return NULL;
-    }
-
-    memcpy(result, s, slen + 1);
-    return result;
+    uint32_t capacity_mask = map->capacity - 1;
+    return hash & capacity_mask;
 }
diff --git a/tests/unit/map_test.c b/tests/unit/map_test.c
index 3eb9acd..fab4c69 100644
--- a/tests/unit/map_test.c
+++ b/tests/unit/map_test.c
@@ -52,7 +52,10 @@ test_map_put_and_get(const MunitParameter params[], void *user_data_or_fixture)
     assert_int(*((int *)map_get(map, "n1")), ==, n1);
     assert_int(*((int *)map_get(map, "n2")), ==, n2);
 
+    map_put(map, "n1", (void *)&n2);
+
     arena_free(&arena);
+    assert_int(*((int *)map_get(map, "n1")), ==, n2);
 
     return MUNIT_OK;
 }