Fix the hashmap_remove flaw.

.github/workflows/cmake.yml

@@ -14,7 +14,6 @@ jobs:
         type: [Debug, RelWithDebInfo, MinSizeRel, Release]
         compiler: [default, clang, gcc]
         exclude:
-          - {os: "windows-latest", compiler: "clang"}
           - {os: "macOS-latest", compiler: "clang"}
           - {os: "windows-latest", compiler: "gcc"}
           - {os: "macOS-latest", compiler: "gcc"}
@@ -44,12 +43,18 @@ jobs:
       working-directory: ${{github.workspace}}/build
       run: cmake $GITHUB_WORKSPACE/test -DCMAKE_BUILD_TYPE=${{ matrix.type }} -DCMAKE_C_COMPILER=gcc-10 -DCMAKE_CXX_COMPILER=g++-10
 
-    - name: Configure CMake with Clang
+    - name: Configure CMake with Clang (Ubuntu)
       shell: bash
-      if: matrix.compiler == 'clang'
+      if: (matrix.compiler == 'clang') && startsWith(matrix.os, 'ubuntu')
       working-directory: ${{github.workspace}}/build
       run: cmake $GITHUB_WORKSPACE/test -DCMAKE_BUILD_TYPE=${{ matrix.type }} -DCMAKE_C_COMPILER=clang-10 -DCMAKE_CXX_COMPILER=clang++-10
 
+    - name: Configure CMake with Clang (Windows)
+      shell: bash
+      if: (matrix.compiler == 'clang') && startsWith(matrix.os, 'windows')
+      working-directory: ${{github.workspace}}/build
+      run: cmake $GITHUB_WORKSPACE/test -DCMAKE_BUILD_TYPE=${{ matrix.type }} -T ClangCL
+
     - name: Build
       working-directory: ${{github.workspace}}/build
       shell: bash

hashmap.h

@@ -286,6 +286,7 @@ int hashmap_remove(struct hashmap_s *const m, const char *const key,
         return 0;
       }
     }
+
     curr = (curr + 1) % m->table_size;
   }
 
@@ -449,6 +450,7 @@ int hashmap_hash_helper(const struct hashmap_s *const m, const char *const key,
                         const unsigned len, unsigned *const out_index) {
   unsigned int curr;
   unsigned int i;
+  int total_in_use;
 
   /* If full, return immediately */
   if (m->size >= m->table_size) {
@@ -458,22 +460,35 @@ int hashmap_hash_helper(const struct hashmap_s *const m, const char *const key,
   /* Find the best index */
   curr = hashmap_hash_helper_int_helper(m, key, len);
 
-  /* Linear probing */
+  /* First linear probe to check if we've already insert the element */
+  total_in_use = 0;
+
   for (i = 0; i < HASHMAP_MAX_CHAIN_LENGTH; i++) {
-    if (!m->data[curr].in_use) {
-      *out_index = curr;
-      return 1;
-    }
+    const int in_use = m->data[curr].in_use;
+
+    total_in_use += in_use;
 
-    if (m->data[curr].in_use &&
-        hashmap_match_helper(&m->data[curr], key, len)) {
+    if (in_use && hashmap_match_helper(&m->data[curr], key, len)) {
       *out_index = curr;
       return 1;
     }
 
     curr = (curr + 1) % m->table_size;
   }
 
+  /* Second linear probe to actually insert our element (only if there was at
+   * least one empty entry) */
+  if (HASHMAP_MAX_CHAIN_LENGTH > total_in_use) {
+    for (i = 0; i < HASHMAP_MAX_CHAIN_LENGTH; i++) {
+      if (!m->data[curr].in_use) {
+        *out_index = curr;
+        return 1;
+      }
+
+      curr = (curr + 1) % m->table_size;
+    }
+  }
+
   return 0;
 }
 

test/CMakeLists.txt

@@ -24,7 +24,7 @@
 # For more information, please refer to <http://unlicense.org/>
 
 project(hashmap)
-cmake_minimum_required(VERSION 3.1.3)
+cmake_minimum_required(VERSION 3.15)
 
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../)
 
@@ -33,9 +33,15 @@ if("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU")
     COMPILE_FLAGS "-Wall -Wextra -Werror -std=gnu89"
   )
 elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")
-  set_source_files_properties(main.c test.c PROPERTIES
-    COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=gnu89"
-  )
+  if(CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC")
+    set_source_files_properties(main.c test.c PROPERTIES
+      COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045"
+    )
+  else()
+    set_source_files_properties(main.c test.c PROPERTIES
+      COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=gnu89"
+    )
+  endif()
 elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")
   set_source_files_properties(main.c test.c PROPERTIES
     COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045"
@@ -49,9 +55,15 @@ if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
     COMPILE_FLAGS "-Wall -Wextra -Werror -std=gnu++98"
   )
 elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
-  set_source_files_properties(test.cpp PROPERTIES
-    COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=gnu++98"
-  )
+  if(CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC")
+    set_source_files_properties(test.cpp PROPERTIES
+      COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045"
+    )
+  else()
+    set_source_files_properties(test.cpp PROPERTIES
+      COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=gnu++98"
+    )
+  endif()
 elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
   set_source_files_properties(test.cpp PROPERTIES
     COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045"
@@ -65,9 +77,15 @@ if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
     COMPILE_FLAGS "-Wall -Wextra -Werror -std=c++11"
   )
 elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
-  set_source_files_properties(test11.cpp PROPERTIES
-    COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=c++11 -Wno-c++98-compat"
-  )
+  if(CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC")
+    set_source_files_properties(test11.cpp PROPERTIES
+      COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045"
+    )
+  else()
+    set_source_files_properties(test11.cpp PROPERTIES
+      COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=c++11"
+    )
+  endif()
 elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
   set_source_files_properties(test11.cpp PROPERTIES
     COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045"
@@ -82,9 +100,15 @@ if (NOT "${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "arm64")
       COMPILE_FLAGS "-Wall -Wextra -Werror -std=gnu89 -msse4.2"
     )
   elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")
-    set_source_files_properties(test_sse42.c PROPERTIES
-      COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=gnu89 -msse4.2"
-    )
+    if(CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC")
+      set_source_files_properties(test_sse42.c PROPERTIES
+        COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045 /arch:AVX"
+      )
+    else()
+      set_source_files_properties(test_sse42.c PROPERTIES
+        COMPILE_FLAGS "-Wall -Wextra -Weverything -Werror -std=gnu89 -msse4.2"
+      )
+    endif()
   elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")
     set_source_files_properties(test_sse42.c PROPERTIES
       COMPILE_FLAGS "/Wall /WX /wd4514 /wd5045 /arch:AVX"

test/test.c

@@ -191,3 +191,31 @@ unsigned crc32_scalar(const char *const s, const unsigned len);
 unsigned crc32_scalar(const char *const s, const unsigned len) {
   return hashmap_crc32_helper(s, len);
 }
+
+UTEST(c, hash_conflict) {
+  struct hashmap_s hashmap;
+
+  int x = 42;
+  int y = 13;
+  int z = -53;
+
+  ASSERT_EQ(0, hashmap_create(4, &hashmap));
+
+  // These all hash to the same value.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "000", 3, &x));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "002", 3, &y));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(3u, hashmap_num_entries(&hashmap));
+
+  // Now we remove the middle value.
+  ASSERT_EQ(0, hashmap_remove(&hashmap, "002", 3));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  // And now attempt to insert the last value again. There was a bug where this
+  // would insert a new entry incorrectly instead of resolving to the previous
+  // entry.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  hashmap_destroy(&hashmap);
+}

test/test.cpp

@@ -158,3 +158,31 @@ UTEST(cpp, num_entries) {
   ASSERT_EQ(0u, hashmap_num_entries(&hashmap));
   hashmap_destroy(&hashmap);
 }
+
+UTEST(cpp, hash_conflict) {
+  struct hashmap_s hashmap;
+
+  int x = 42;
+  int y = 13;
+  int z = -53;
+
+  ASSERT_EQ(0, hashmap_create(4, &hashmap));
+
+  // These all hash to the same value.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "000", 3, &x));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "002", 3, &y));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(3u, hashmap_num_entries(&hashmap));
+
+  // Now we remove the middle value.
+  ASSERT_EQ(0, hashmap_remove(&hashmap, "002", 3));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  // And now attempt to insert the last value again. There was a bug where this
+  // would insert a new entry incorrectly instead of resolving to the previous
+  // entry.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  hashmap_destroy(&hashmap);
+}

test/test11.cpp

@@ -1,15 +1,19 @@
 #include "hashmap.h"
 #include "utest.h"
 
-// Old MSVC doesn't support noexcept
-#if defined(_MSC_VER) && (_MSC_VER < 1900)
+#if defined(_MSC_VER) && (_MSC_VER < 1920)
 #define NOTHROW __declspec(nothrow)
 #define NOEXCEPT
 #else
 #define NOTHROW
 #define NOEXCEPT noexcept
 #endif
 
+#if defined(__clang__)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++98-compat"
+#endif
+
 UTEST(cpp11, create) {
   struct hashmap_s hashmap;
   ASSERT_EQ(0, hashmap_create(1, &hashmap));
@@ -163,3 +167,31 @@ UTEST(cpp11, num_entries) {
   ASSERT_EQ(0u, hashmap_num_entries(&hashmap));
   hashmap_destroy(&hashmap);
 }
+
+UTEST(cpp11, hash_conflict) {
+  struct hashmap_s hashmap;
+
+  int x = 42;
+  int y = 13;
+  int z = -53;
+
+  ASSERT_EQ(0, hashmap_create(4, &hashmap));
+
+  // These all hash to the same value.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "000", 3, &x));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "002", 3, &y));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(3u, hashmap_num_entries(&hashmap));
+
+  // Now we remove the middle value.
+  ASSERT_EQ(0, hashmap_remove(&hashmap, "002", 3));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  // And now attempt to insert the last value again. There was a bug where this
+  // would insert a new entry incorrectly instead of resolving to the previous
+  // entry.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  hashmap_destroy(&hashmap);
+}

test/test_sse42.c

@@ -160,4 +160,32 @@ UTEST(c_sse42, crc32_matches) {
   }
 }
 
+UTEST(c_sse42, hash_conflict) {
+  struct hashmap_s hashmap;
+
+  int x = 42;
+  int y = 13;
+  int z = -53;
+
+  ASSERT_EQ(0, hashmap_create(4, &hashmap));
+
+  // These all hash to the same value.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "000", 3, &x));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "002", 3, &y));
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(3u, hashmap_num_entries(&hashmap));
+
+  // Now we remove the middle value.
+  ASSERT_EQ(0, hashmap_remove(&hashmap, "002", 3));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  // And now attempt to insert the last value again. There was a bug where this
+  // would insert a new entry incorrectly instead of resolving to the previous
+  // entry.
+  ASSERT_EQ(0, hashmap_put(&hashmap, "007", 3, &z));
+  ASSERT_EQ(2u, hashmap_num_entries(&hashmap));
+
+  hashmap_destroy(&hashmap);
+}
+
 #endif