From de10a989585be4b4d8af4a5b69ceaff3b9f6a801 Mon Sep 17 00:00:00 2001
From: Rain Valentine <rsg000@gmail.com>
Date: Fri, 25 Oct 2024 22:06:11 +0000
Subject: [PATCH] Fixed a UT failure that only happened with O3 optimization

Signed-off-by: Rain Valentine <rsg000@gmail.com>
---
 src/unit/test_hashset.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/unit/test_hashset.c b/src/unit/test_hashset.c
index c27d6b3edd..3aa63af5e1 100644
--- a/src/unit/test_hashset.c
+++ b/src/unit/test_hashset.c
@@ -425,10 +425,9 @@ int test_safe_iterator(int argc, char **argv, int flags) {
     /* A set of longs, i.e. pointer-sized values. */
     hashsetType type = {0};
     hashset *s = hashsetCreate(&type);
-    long j;
 
     /* Populate */
-    for (j = 0; j < count; j++) {
+    for (long j = 0; j < count; j++) {
         assert(hashsetAdd(s, (void *)j));
     }
 
@@ -438,7 +437,26 @@ int test_safe_iterator(int argc, char **argv, int flags) {
     long num_returned = 0;
     hashsetIterator iter;
     hashsetInitSafeIterator(&iter, s);
-    while (hashsetNext(&iter, (void **)&j)) {
+
+    /* Important note for directly storing data instead of pointers in hashset:
+     * This code will probably fail if compiled with -O3, which may reorder
+     * statements:
+     *
+     * long j = 1000;
+     * while (hashsetNext(&iter, (void **)&j)) {
+     *
+     * The cast from long* to void** confuses the compiler, which fails to
+     * recognize that hashsetNext may change the value of j. In this case it
+     * reordered hashsetDelete() before hashsetNext() and attempted to delete
+     * element 1000, which does not exist.
+     * 
+     * When storing something other than a pointer in hashset, an intermediary 
+     * variable (example below) avoids the problematic cast and clarifies our
+     * intention for the compiler. */
+    void* element;
+    while (hashsetNext(&iter, &element)) {
+        long j = (long) element;
+
         num_returned++;
         if (j < 0 || j >= count * 2) {
             printf("Element %ld returned, max == %ld. Num returned: %ld\n", j, count * 2 - 1, num_returned);
@@ -462,7 +480,7 @@ int test_safe_iterator(int argc, char **argv, int flags) {
     /* Check that all elements present during the whole iteration were returned
      * exactly once. (Some are deleted after being returned.) */
     TEST_ASSERT(num_returned >= count);
-    for (j = 0; j < count; j++) {
+    for (long j = 0; j < count; j++) {
         if (element_returned[j] != 1) {
             printf("Element %ld returned %d times\n", j, element_returned[j]);
             return 0;
@@ -471,7 +489,7 @@ int test_safe_iterator(int argc, char **argv, int flags) {
     /* Check that elements inserted during the iteration were returned at most
      * once. */
     unsigned long num_optional_returned;
-    for (j = count; j < count * 2; j++) {
+    for (long j = count; j < count * 2; j++) {
         assert(element_returned[j] <= 1);
         num_optional_returned += element_returned[j];
     }