Attached is a patch that that adds an abstraction to commonly used functionality in the code base - to iterate raw hashes.

It is implemented as a macros:
1st macro adds syntax like:

parrot_hash_iterate(hash, <some_code_here>);

in the code, there is defined _bucket variable;

for example

parrot_hash_iterate(hash, _bucket->value += 1);

The code could be of arbitrary size;

the second macro "parrot_hash_iterator" is modeled after hashiterator.pmc. For example usage, look there.

I have replaced all manual iterations of hashes with this macros. This will give us a flexibility to change hash internals only changing 2 files - hash.c and hash.h whithout breaking unknown places that poke with hash internals.

As macros, they are text substitution, so there will be no performace hit.

I am open to comments and suggestions

luben


Index: src/hash.c
===================================================================
--- src/hash.c	(revision 48455)
+++ src/hash.c	(working copy)
@@ -512,22 +512,10 @@
 parrot_mark_hash_keys(PARROT_INTERP, ARGIN(Hash *hash))
 {
     ASSERT_ARGS(parrot_mark_hash_keys)
-    const UINTVAL entries = hash->entries;
-    UINTVAL found = 0;
-    UINTVAL i;
-
-    HashBucket *bucket = hash->buckets;
-
-    for (i= 0; i <= hash->mask; ++i, ++bucket) {
-        if (bucket->key){
-
-            PARROT_ASSERT(bucket->key);
-            Parrot_gc_mark_PObj_alive(interp, (PObj *)bucket->key);
-
-            if (++found >= entries)
-                break;
-        }
-    }
+    parrot_hash_iterate(hash,
+        PARROT_ASSERT(_bucket->key);
+        Parrot_gc_mark_PObj_alive(interp, (PObj *)_bucket->key);
+    );
 }
 
 
@@ -545,24 +533,13 @@
 parrot_mark_hash_values(PARROT_INTERP, ARGIN(Hash *hash))
 {
     ASSERT_ARGS(parrot_mark_hash_values)
-    const UINTVAL entries = hash->entries;
-    UINTVAL found = 0;
-    UINTVAL i;
-
-    HashBucket *bucket = hash->buckets;
-
-    for (i= 0; i <= hash->mask; ++i, ++bucket) {
-        if (bucket->key){
-
-            PARROT_ASSERT(bucket->value);
-            Parrot_gc_mark_PObj_alive(interp, (PObj *)bucket->value);
-
-            if (++found >= entries)
-                break;
-        }
-    }
+    parrot_hash_iterate(hash,
+        PARROT_ASSERT(_bucket->value);
+        Parrot_gc_mark_PObj_alive(interp, (PObj *)_bucket->value);
+    );
 }
 
+
 /*
 
 =item C<static void parrot_mark_hash_both(PARROT_INTERP, Hash *hash)>
Index: src/pmc/lexinfo.pmc
===================================================================
--- src/pmc/lexinfo.pmc	(revision 48455)
+++ src/pmc/lexinfo.pmc	(working copy)
@@ -99,26 +99,12 @@
         if (Parrot_str_equal(INTERP, what, CONST_STRING(INTERP, "symbols"))) {
             PMC * const result    = Parrot_pmc_new(INTERP, enum_class_ResizableStringArray);
             const Hash *hash      = (Hash *)SELF.get_pointer();
-            const UINTVAL entries = hash->entries;
 
-            UINTVAL found   = 0;
-            INTVAL  i;
+            parrot_hash_iterate(hash,
+                PARROT_ASSERT(_bucket->key);
+                VTABLE_push_string(INTERP, result, (STRING *)_bucket->key);
+            );
 
-            for (i = hash->mask; i >= 0; --i) {
-                HashBucket *bucket = hash->bucket_indices[i];
-                while (bucket) {
-                    if (++found > entries)
-                        Parrot_ex_throw_from_c_args(INTERP, NULL, 1,
-                            "Detected corruption at LexInfo hash %p entries %d",
-                            hash, (int)entries);
-
-                    PARROT_ASSERT(bucket->key);
-                    VTABLE_push_string(INTERP, result, (STRING *)bucket->key);
-
-                    bucket = bucket->next;
-                }
-            }
-
             return result;
         }
         else
Index: src/pmc/hash.pmc
===================================================================
--- src/pmc/hash.pmc	(revision 48455)
+++ src/pmc/hash.pmc	(working copy)
@@ -451,12 +451,6 @@
         PMC    *box;
         HashBucket *b;
 
-        if (PObj_constant_TEST(SELF)
-        && !PObj_constant_TEST((PObj *)key))
-            Parrot_ex_throw_from_c_args(INTERP, NULL,
-                EXCEPTION_INVALID_OPERATION,
-                "Used non-constant PMC key in constant hash.");
-
         if (!nextkey) {
             parrot_hash_put(INTERP, hash, keystr,
                     hash_value_from_int(INTERP, hash, value));
@@ -644,16 +638,11 @@
         PMC    *box;
         HashBucket *b;
 
-        if (PObj_constant_TEST(SELF)){
-            if (!PObj_constant_TEST((PObj *)key))
-                Parrot_ex_throw_from_c_args(INTERP, NULL,
-                    EXCEPTION_INVALID_OPERATION,
-                    "Used non-constant PMC key in constant hash.");
-            if (!PObj_constant_TEST((PObj *)value))
-                Parrot_ex_throw_from_c_args(INTERP, NULL,
-                    EXCEPTION_INVALID_OPERATION,
-                    "Used non-constant STRING value in constant hash.");
-        }
+        if (PObj_constant_TEST(SELF)
+        && (!PObj_constant_TEST((PObj *)value)))
+            Parrot_ex_throw_from_c_args(INTERP, NULL,
+                EXCEPTION_INVALID_OPERATION,
+                "Used non-constant STRING value in constant hash.");
 
         if (!nextkey) {
             parrot_hash_put(INTERP, hash, keystr,
@@ -800,12 +789,6 @@
         PMC    *box            = PMCNULL;
         HashBucket *b;
 
-        if (PObj_constant_TEST(SELF)
-        && !PObj_constant_TEST((PObj *)key))
-            Parrot_ex_throw_from_c_args(INTERP, NULL,
-                EXCEPTION_INVALID_OPERATION,
-                "Used non-constant PMC key in constant hash.");
-
         if (!nextkey) {
             PMC * const val = get_number_pmc(INTERP, value);
             parrot_hash_put(INTERP, hash, keystr, hash_value_from_pmc(INTERP, hash, val));
@@ -860,18 +843,12 @@
         PMC    *box;
         HashBucket *b;
 
-        if (PObj_constant_TEST(SELF)) {
-            if (!PObj_constant_TEST((PObj *)key))
-                Parrot_ex_throw_from_c_args(INTERP, NULL,
-                    EXCEPTION_INVALID_OPERATION,
-                    "Used non-constant PMC key in constant hash.");
+        if (PObj_constant_TEST(SELF)
+        && (!PObj_constant_TEST((PObj *)value)))
+            Parrot_ex_throw_from_c_args(INTERP, NULL,
+                EXCEPTION_INVALID_OPERATION,
+                "Used non-constant PMC value in constant hash.");
 
-            if (!PObj_constant_TEST((PObj *)value))
-                Parrot_ex_throw_from_c_args(INTERP, NULL,
-                    EXCEPTION_INVALID_OPERATION,
-                    "Used non-constant PMC value in constant hash.");
-        }
-
         if (!nextkey) {
             parrot_hash_put(INTERP, hash, keystr, value);
             return;
Index: src/pmc/callcontext.pmc
===================================================================
--- src/pmc/callcontext.pmc	(revision 48455)
+++ src/pmc/callcontext.pmc	(working copy)
@@ -378,17 +378,10 @@
 mark_hash(PARROT_INTERP, ARGIN(Hash *h))
 {
     ASSERT_ARGS(mark_hash)
-    INTVAL  i;
-
-    for (i = h->mask; i >= 0; --i) {
-        HashBucket *b = h->bucket_indices[i];
-
-        while (b) {
-            Parrot_gc_mark_STRING_alive(interp, (STRING *)b->key);
-            mark_cell(interp, (Pcc_cell *)b->value);
-            b = b->next;
-        }
-    }
+    parrot_hash_iterate(h,
+        Parrot_gc_mark_STRING_alive(interp, (STRING *)_bucket->key);
+        mark_cell(interp, (Pcc_cell *)_bucket->value);
+    );
 }
 
 PARROT_CAN_RETURN_NULL
@@ -402,19 +395,11 @@
 
     /* yes, this *looks* risky, but it's a Parrot STRING hash internally */
     if (hash && hash->entries) {
-        UINTVAL i, j = 0;
+        UINTVAL j = 0;
         PMC *result = Parrot_pmc_new_init_int(interp, enum_class_FixedStringArray, hash->entries);
-
-        for (i = 0; i <= hash->mask; ++i) {
-            HashBucket *b = hash->bucket_indices[i];
-
-            while (b) {
-                VTABLE_set_string_keyed_int(interp, result,
-                    j++, (STRING *)b->key);
-                b = b->next;
-            }
-        }
-
+        parrot_hash_iterate(hash,
+            VTABLE_set_string_keyed_int(interp, result, j++, (STRING *)_bucket->key);
+        );
         return result;
     }
 
@@ -604,17 +589,9 @@
         GET_ATTR_hash(INTERP, SELF, hash);
 
         if (hash) {
-            UINTVAL i;
-
-            for (i = 0; i <= hash->mask; ++i) {
-                HashBucket *b = hash->bucket_indices[i];
-
-                while (b) {
-                    FREE_CELL(INTERP, (Pcc_cell *)b->value);
-                    b = b->next;
-                }
-            }
-
+            parrot_hash_iterate(hash,
+               FREE_CELL(INTERP, (Pcc_cell *)_bucket->value);
+            );
             parrot_hash_destroy(INTERP, hash);
             SET_ATTR_hash(INTERP, SELF, NULL);
         }
@@ -642,17 +619,9 @@
         }
 
         if (hash) {
-            UINTVAL i;
-
-            for (i = 0; i <= hash->mask; ++i) {
-                HashBucket *b = hash->bucket_indices[i];
-
-                while (b) {
-                    FREE_CELL(INTERP, (Pcc_cell *)b->value);
-                    b = b->next;
-                }
-            }
-
+            parrot_hash_iterate(hash,
+                FREE_CELL(INTERP, (Pcc_cell *)_bucket->value);
+            );
             parrot_hash_destroy(INTERP, hash);
         }
 
Index: src/pmc/hashiterator.pmc
===================================================================
--- src/pmc/hashiterator.pmc	(revision 48455)
+++ src/pmc/hashiterator.pmc	(working copy)
@@ -78,24 +78,9 @@
 advance_to_next(PARROT_INTERP, ARGMOD(PMC *self))
 {
     ASSERT_ARGS(advance_to_next)
-
     Parrot_HashIterator_attributes * const attrs  = PARROT_HASHITERATOR(self);
-    HashBucket                            *bucket = attrs->bucket;
-
-    /* Try to advance current bucket */
-    if (bucket)
-        bucket = bucket->next;
-
-    while (!bucket) {
-        /* If there is no more buckets */
-        if (attrs->pos == attrs->total_buckets)
-            break;
-
-        bucket = attrs->parrot_hash->bucket_indices[attrs->pos++];
-    }
-    attrs->bucket = bucket;
+    parrot_hash_iterator(attrs->parrot_hash, attrs->bucket, attrs->pos)
     --attrs->elements;
-
     return;
 }
 
Index: src/packfile.c
===================================================================
--- src/packfile.c	(revision 48455)
+++ src/packfile.c	(working copy)
@@ -3471,22 +3471,12 @@
     if (!hash)
         return;
 
-    for (i = 0; i <= hash->mask; ++i) {
-        HashBucket *bucket = hash->bucket_indices[i];
-
-        while (bucket) {
-            PackFile_ConstTable * const table      =
-                (PackFile_ConstTable *)bucket->key;
-            PackFile_Constant * const orig_consts = table->constants;
-            PackFile_Constant * const consts      =
-                (PackFile_Constant *) bucket->value;
-            INTVAL j;
-
-            mem_gc_free(interp, consts);
-            bucket = bucket->next;
-        }
-    }
-
+    parrot_hash_iterate(hash,
+        PackFile_ConstTable * const table     = (PackFile_ConstTable *)_bucket->key;
+        PackFile_Constant * const orig_consts = table->constants;
+        PackFile_Constant * const consts      = (PackFile_Constant *) _bucket->value;
+        mem_gc_free(interp, consts);
+    );
     parrot_hash_destroy(interp, hash);
 }
 
Index: include/parrot/hash.h
===================================================================
--- include/parrot/hash.h	(revision 48455)
+++ include/parrot/hash.h	(working copy)
@@ -78,6 +78,33 @@
     hash_hash_key_fn hash_val;
 };
 
+/* Utility macros - use them, do not reinvent the weel */
+#define parrot_hash_iterate(_hash, _code)                                   \
+{                                                                           \
+    INTVAL _hash_i;                                                         \
+    for (_hash_i = (_hash)->mask; _hash_i >= 0; --_hash_i) {                \
+        HashBucket *_bucket = (_hash)->bucket_indices[_hash_i];             \
+        while (_bucket) {                                                   \
+            _code                                                           \
+            _bucket = _bucket->next;                                        \
+        }                                                                   \
+    }                                                                       \
+}
+
+#define parrot_hash_iterator(_hash,_bucket,_pos)                            \
+{                                                                           \
+    /* Try to advance current bucket */                                     \
+    if ((_bucket))                                                          \
+        (_bucket) = (_bucket)->next;                                        \
+    while (!(_bucket)) {                                                    \
+        /* If there is no more buckets */                                   \
+        if ((_pos) == (INTVAL)(_hash)->mask+1)                              \
+            break;                                                          \
+        (_bucket) = (_hash)->bucket_indices[_pos++];                        \
+    }                                                                       \
+}
+
+
 typedef void (*value_free)(ARGFREE(void *));
 
 /* To avoid creating OrderedHashItem PMC we reuse FixedPMCArray PMC */

<<attachment: luben.vcf>>

_______________________________________________
http://lists.parrot.org/mailman/listinfo/parrot-dev

Reply via email to