#509: Fix celix string/long hashmap equals functions

apache · Nov 6, 2023 · b5f25b5 · b5f25b5
1 parent 833a5c8
commit b5f25b5
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 13 deletions.
diff --git a/libs/utils/gtest/src/HashMapTestSuite.cc b/libs/utils/gtest/src/HashMapTestSuite.cc
@@ -615,11 +615,15 @@ TEST_F(HashMapTestSuite, StoreKeysWeaklyTest) {
         free(key);
     };
     auto* sMap = celix_stringHashMap_createWithOptions(&opts);
-    EXPECT_FALSE(celix_stringHashMap_putLong(sMap, celix_utils_strdup("key1"), 1)); // new key -> takes ownership
-    EXPECT_TRUE(celix_stringHashMap_putLong(sMap, "key1", 2));                      // replace key -> takes no ownership
-
-    EXPECT_FALSE(celix_stringHashMap_putLong(sMap, celix_utils_strdup("key2"), 3)); // new key -> takes ownership
-    EXPECT_TRUE(celix_stringHashMap_putLong(sMap, "key2", 4));                      // replace key -> takes no ownership
+    EXPECT_FALSE(celix_stringHashMap_hasKey(sMap, "key1"));
+    EXPECT_EQ(CELIX_SUCCESS, celix_stringHashMap_putLong(sMap, celix_utils_strdup("key1"), 1)); // new key -> takes ownership
+    EXPECT_TRUE(celix_stringHashMap_hasKey(sMap, "key1"));
+    EXPECT_EQ(CELIX_SUCCESS, celix_stringHashMap_putLong(sMap, "key1", 2)); // replace key -> takes no ownership
+
+    EXPECT_FALSE(celix_stringHashMap_hasKey(sMap, "key2"));
+    EXPECT_EQ(CELIX_SUCCESS, celix_stringHashMap_putLong(sMap, celix_utils_strdup("key2"), 3)); // new key -> takes ownership
+    EXPECT_TRUE(celix_stringHashMap_hasKey(sMap, "key2"));
+    EXPECT_EQ(CELIX_SUCCESS, celix_stringHashMap_putLong(sMap, "key2", 4)); // replace key -> takes no ownership
     celix_stringHashMap_remove(sMap, "key1");
 
     celix_stringHashMap_destroy(sMap);
@@ -639,6 +643,9 @@ TEST_F(HashMapTestSuite, StringHashMapEqualsTest) {
     celix_autoptr(celix_string_hash_map_t) map1 = celix_stringHashMap_create();
     celix_autoptr(celix_string_hash_map_t) map2 = celix_stringHashMap_create();
     EXPECT_TRUE(celix_stringHashMap_equals(map1, map2));
+    EXPECT_FALSE(celix_stringHashMap_equals(map1, nullptr));
+    EXPECT_FALSE(celix_stringHashMap_equals(nullptr, map2));
+    EXPECT_TRUE(celix_stringHashMap_equals(nullptr, nullptr));
 
     celix_stringHashMap_putLong(map1, "key1", 42);
     celix_stringHashMap_putBool(map1, "key2", true);
@@ -657,6 +664,9 @@ TEST_F(HashMapTestSuite, LongHashMapEqualsTest) {
     celix_autoptr(celix_long_hash_map_t) map1 = celix_longHashMap_create();
     celix_autoptr(celix_long_hash_map_t) map2 = celix_longHashMap_create();
     EXPECT_TRUE(celix_longHashMap_equals(map1, map2));
+    EXPECT_FALSE(celix_longHashMap_equals(map1, nullptr));
+    EXPECT_FALSE(celix_longHashMap_equals(nullptr, map2));
+    EXPECT_TRUE(celix_longHashMap_equals(nullptr, nullptr));
 
     celix_longHashMap_putLong(map1, 1, 42);
     celix_longHashMap_putBool(map1, 2, true);

diff --git a/libs/utils/include/celix_string_hash_map.h b/libs/utils/include/celix_string_hash_map.h
@@ -319,6 +319,8 @@ CELIX_UTILS_EXPORT void celix_stringHashMap_clear(celix_string_hash_map_t* map);
  * Equals means that both maps have the same number of entries and that all entries in the first map
  * are also present in the second map and have the same value.
  *
+ * The value are compared using memcpy, so for pointer values the pointer value is compared and not the value itself.
+ *
  * @param[in] map1 The first map to compare.
  * @param[in] map2 The second map to compare.
  * @return true if the maps are equal, false otherwise.
@@ -359,6 +361,8 @@ CELIX_UTILS_EXPORT void celix_stringHashMapIterator_next(celix_string_hash_map_i
 /**
  * @brief Compares two celix_string_hash_map_iterator_t objects for equality.
  *
+ * The value are compared using memcpy, so for pointer values the pointer value is compared and not the value itself.
+ *
  * @param[in] iterator The first iterator to compare.
  * @param[in] other The second iterator to compare.
  * @return true if the iterators point to the same entry in the same hash map, false otherwise.

diff --git a/libs/utils/src/celix_hash_map.c b/libs/utils/src/celix_hash_map.c
@@ -592,29 +592,40 @@ void celix_longHashMap_clear(celix_long_hash_map_t* map) {
 }
 
 static bool celix_hashMap_equals(const celix_hash_map_t* map1, const celix_hash_map_t* map2) {
-    if (map1 == NULL && map2 == NULL) {
-        return true;
-    }
-    if (map1 == NULL || map2 == NULL) {
-            return false;
-    }
     if (map1->size != map2->size) {
         return false;
     }
     for (celix_hash_map_entry_t* entry = celix_hashMap_firstEntry(map1); entry != NULL; entry = celix_hashMap_nextEntry(map1, entry)) {
-        void* value = celix_hashMap_get(map2, entry->key.strKey, entry->key.longKey);
-        if (value == NULL || memcmp(&entry->value, value, sizeof(entry->value)) != 0) {
+        celix_hash_map_entry_t* entryMap2 = celix_hashMap_getEntry(map2, entry->key.strKey, entry->key.longKey);
+
+        //note using memcmp, so for void* values the pointer value is compared, not the value itself.
+        if (entryMap2 == NULL || memcmp(&entryMap2->value, &entry->value, sizeof(entryMap2->value)) != 0) {
             return false;
         }
     }
     return true;
 }
 
 bool celix_stringHashMap_equals(const celix_string_hash_map_t* map1, const celix_string_hash_map_t* map2) {
+    if (map1 == NULL && map2 == NULL) {
+        return true;
+    }
+    if (map1 == NULL || map2 == NULL) {
+        return false;
+    }
     return celix_hashMap_equals(&map1->genericMap, &map2->genericMap);
 }
 
 bool celix_longHashMap_equals(const celix_long_hash_map_t* map1, const celix_long_hash_map_t* map2) {
+    if (map1 == NULL && map2 == NULL) {
+        return true;
+    }
+    if (map1 == NULL || map2 == NULL) {
+        return false;
+    }
+    if (map1->genericMap.size != map2->genericMap.size) {
+        return false;
+    }
     return celix_hashMap_equals(&map1->genericMap, &map2->genericMap);
 }