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

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

Fixing the suggestions based on Johnny's comments:

- on map_test I was asserting after freeing the memory
- on map.c there was a bug where I was valdating that the entry was NULL
  not the entry->key which was leading to always having an empty first
  entry (not a SEGFAULT).
- It does not makes sense to have strdup on arena.
  - Instead I just kept the convention to have a single _.

If you think we are good to go, please send a v2. So it will trigger the
pipeline.

--->8---
diff --git a/src/map.c b/src/map.c
index 532ba3b..6665e18 100644
--- a/src/map.c
+++ b/src/map.c
@@ -32,7 +32,10 @@ static void
 map_init(map_t *map);
 
 static char *
-__strdup(const char *s, arena_t *arena);
+_strdup(const char *s, arena_t *arena);
+
+static uint32_t
+map_get_index(map_t *map, uint32_t hash);
 
 map_t *
 map_new(arena_t *arena)
@@ -78,24 +81,26 @@ 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_get_index(map, hash);
 
-    while (entry != NULL) {
+    if (entry->key == NULL) {
+        *entry = (map_entry_t){ .key = _strdup(key, map->arena), .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 = _strdup(key, map->arena), .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 +109,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_get_index(map, hash);
     while (entry != NULL) {
         if (entry->hash == hash && strcmp(entry->key, key) == 0) {
             return entry->value;
@@ -114,8 +119,15 @@ map_get(map_t *map, char *key)
     return NULL;
 }
 
+static uint32_t
+map_get_index(map_t *map, uint32_t hash)
+{
+    uint32_t capacity_mask = map->capacity - 1;
+    return hash & capacity_mask;
+}
+
 static char *
-__strdup(const char *s, arena_t *arena)
+_strdup(const char *s, arena_t *arena)
 {
     size_t slen = strlen(s);
     char *result = arena_alloc(arena, slen + 1);
diff --git a/tests/unit/map_test.c b/tests/unit/map_test.c
index 3eb9acd..449bca6 100644
--- a/tests/unit/map_test.c
+++ b/tests/unit/map_test.c
@@ -52,6 +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);
+
+    assert_int(*((int *)map_get(map, "n1")), ==, n2);
+
     arena_free(&arena);
 
     return MUNIT_OK;