pr feedback

Signed-off-by: Rain Valentine <[email protected]>

src/db.c

@@ -1086,37 +1086,35 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
      * just return everything inside the object in a single call, setting the
      * cursor to zero to signal the end of the iteration. */
 
-    /* Handle the case of a dict or hashset. */
-    dict *dictTable = NULL;
-    hashset *hashsetTable = NULL;
+    /* Handle the case of kvstore, dict or hashset. */
+    dict *dict_table = NULL;
+    hashset *hashset_table = NULL;
+    int shallow_copied_list_items = 0;
     if (o == NULL) {
-        dictTable = NULL;
+        shallow_copied_list_items = 1;
     } else if (o->type == OBJ_SET && o->encoding == OBJ_ENCODING_HASHSET) {
-        hashsetTable = o->ptr;
+        hashset_table = o->ptr;
+        shallow_copied_list_items = 1;
     } else if (o->type == OBJ_HASH && o->encoding == OBJ_ENCODING_HT) {
-        dictTable = o->ptr;
+        dict_table = o->ptr;
+        shallow_copied_list_items = 1;
     } else if (o->type == OBJ_ZSET && o->encoding == OBJ_ENCODING_SKIPLIST) {
         zset *zs = o->ptr;
-        dictTable = zs->dict;
+        dict_table = zs->dict;
+        /* scanning ZSET allocates temporary strings even though it's a dict */
+        shallow_copied_list_items = 0;
     }
 
     list *keys = listCreate();
-    /* Set a free callback for the contents of the collected keys list.
-     * For the main keyspace dict, when we scan a key that's dict encoded
-     * (we have 'dictTable'), or when we scan a key that's hashset encoded
-     * (we have 'hashsetTable') we don't need to define free method because the
-     * strings in the list are just a shallow copy from the pointer in the
-     * dictEntry.
-     * When scanning a key with other encodings (e.g. listpack), we need to
-     * free the temporary strings we add to that list.
-     * The exception to the above is ZSET, where we do allocate temporary
-     * strings even when scanning a dict. */
-    if (o && ((!dictTable && !hashsetTable) || o->type == OBJ_ZSET)) {
+    /* Set a free callback for the contents of the collected keys list if they
+     * are deep copied temporary strings. We must not free them if they are just
+     * a shallow copy - a pointer to the actual data in the data structure */
+    if (!shallow_copied_list_items) {
         listSetFreeMethod(keys, (void (*)(void *))sdsfree);
     }
 
-    /* For main dictionary scan or data structure using hashtable. */
-    if (!o || dictTable || hashsetTable) {
+    /* For main dictionary scan or scannable data structure. */
+    if (!o || dict_table || hashset_table) {
         /* We set the max number of iterations to ten times the specified
          * COUNT, so if the hash table is in a pathological state (very
          * sparsely populated) we avoid to block too much time at the cost
@@ -1155,10 +1153,10 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
              * If cursor is empty, we should try exploring next non-empty slot. */
             if (o == NULL) {
                 cursor = kvstoreScan(c->db->keys, cursor, onlydidx, dictScanCallback, NULL, &data);
-            } else if (dictTable) {
-                cursor = dictScan(dictTable, cursor, dictScanCallback, &data);
+            } else if (dict_table) {
+                cursor = dictScan(dict_table, cursor, dictScanCallback, &data);
             } else {
-                cursor = hashsetScan(hashsetTable, cursor, hashsetScanCallback, &data, 0);
+                cursor = hashsetScan(hashset_table, cursor, hashsetScanCallback, &data, 0);
             }
         } while (cursor && maxiterations-- && data.sampled < count);
     } else if (o->type == OBJ_SET) {

src/t_set.c

@@ -245,7 +245,7 @@ int setTypeRemoveAux(robj *setobj, char *str, size_t len, int64_t llval, int str
 
     if (setobj->encoding == OBJ_ENCODING_HASHSET) {
         sds sdsval = str_is_sds ? (sds)str : sdsnewlen(str, len);
-        int deleted = (hashsetDelete(setobj->ptr, sdsval));
+        int deleted = hashsetDelete(setobj->ptr, sdsval);
         if (sdsval != str) sdsfree(sdsval); /* free temp copy */
         return deleted;
     } else if (setobj->encoding == OBJ_ENCODING_LISTPACK) {

src/t_zset.c

@@ -2170,7 +2170,7 @@ int zuiNext(zsetopsrc *op, zsetopval *val) {
             /* Move to next element. */
             it->is.ii++;
         } else if (op->encoding == OBJ_ENCODING_HASHSET) {
-            if (!hashsetNext(it->ht.iter, (void **)&(val->ele))) return 0;
+            if (!hashsetNext(it->ht.iter, (void **)&val->ele)) return 0;
             val->score = 1.0;
         } else if (op->encoding == OBJ_ENCODING_LISTPACK) {
             if (it->lp.p == NULL) return 0;