On 13.08.2010 20:18, luben wrote:
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
Here is updated patch that does not have code unrelated to the topic. I
have found some more places where we could use these macros and I am
converting them also.
Index: src/hash.c
===================================================================
--- src/hash.c (revision 48465)
+++ 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)>
@@ -577,24 +554,12 @@
parrot_mark_hash_both(PARROT_INTERP, ARGIN(Hash *hash))
{
ASSERT_ARGS(parrot_mark_hash_both)
- 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);
-
- PARROT_ASSERT(bucket->value);
- Parrot_gc_mark_PObj_alive(interp, (PObj *)bucket->value);
-
- if (++found >= entries)
- break;
- }
- }
+ parrot_hash_iterate(hash,
+ PARROT_ASSERT(_bucket->key);
+ Parrot_gc_mark_PObj_alive(interp, (PObj *)_bucket->key);
+ PARROT_ASSERT(_bucket->value);
+ Parrot_gc_mark_PObj_alive(interp, (PObj *)_bucket->value);
+ );
}
/*
@@ -714,41 +679,38 @@
const size_t entries = hash->entries;
size_t i;
- for (i = 0; i < entries; ++i) {
- HashBucket * const b = hash->buckets + i;
-
+ parrot_hash_iterate(hash,
switch (key_type) {
case Hash_key_type_int:
- VTABLE_push_integer(interp, info, (INTVAL)b->key);
+ VTABLE_push_integer(interp, info, (INTVAL)_bucket->key);
break;
case Hash_key_type_STRING:
- VTABLE_push_string(interp, info, (STRING *)b->key);
+ VTABLE_push_string(interp, info, (STRING *)_bucket->key);
break;
case Hash_key_type_PMC:
- VTABLE_push_pmc(interp, info, (PMC *)b->key);
+ VTABLE_push_pmc(interp, info, (PMC *)_bucket->key);
break;
default:
Parrot_ex_throw_from_c_args(interp, NULL, 1,
"unimplemented key type");
break;
}
-
switch (entry_type) {
case enum_hash_int:
- VTABLE_push_integer(interp, info, (INTVAL)b->value);
+ VTABLE_push_integer(interp, info, (INTVAL)_bucket->value);
break;
case enum_hash_string:
- VTABLE_push_string(interp, info, (STRING *)b->value);
+ VTABLE_push_string(interp, info, (STRING *)_bucket->value);
break;
case enum_hash_pmc:
- VTABLE_push_pmc(interp, info, (PMC *)b->value);
+ VTABLE_push_pmc(interp, info, (PMC *)_bucket->value);
break;
default:
Parrot_ex_throw_from_c_args(interp, NULL, 1,
"unimplemented value type");
break;
}
- }
+ );
}
@@ -1121,17 +1083,10 @@
parrot_chash_destroy(PARROT_INTERP, ARGMOD(Hash *hash))
{
ASSERT_ARGS(parrot_chash_destroy)
- UINTVAL i;
-
- for (i = 0; i <= hash->mask; ++i) {
- HashBucket *bucket = hash->bucket_indices[i];
- while (bucket) {
- mem_gc_free(interp, bucket->key);
- mem_gc_free(interp, bucket->value);
- bucket = bucket->next;
- }
- }
-
+ parrot_hash_iterate(hash,
+ mem_gc_free(interp, _bucket->key);
+ mem_gc_free(interp, _bucket->value);
+ );
parrot_hash_destroy(interp, hash);
}
@@ -1472,33 +1427,30 @@
ARGOUT(Hash *dest), int deep)
{
ASSERT_ARGS(parrot_hash_clone_prunable)
- UINTVAL entries = hash->entries;
- UINTVAL i;
- for (i = 0; i < entries; ++i) {
+ parrot_hash_iterate(hash,
void *valtmp;
- HashBucket *b = hash->buckets + i;
- void * const key = b->key;
+ void * const key = _bucket->key;
switch (hash->entry_type) {
case enum_type_undef:
case enum_type_ptr:
case enum_type_INTVAL:
- valtmp = (void *)b->value;
+ valtmp = (void *)_bucket->value;
break;
case enum_type_STRING:
- valtmp = b->value;
+ valtmp = _bucket->value;
break;
case enum_type_PMC:
- if (PMC_IS_NULL((PMC *)b->value))
+ if (PMC_IS_NULL((PMC *)_bucket->value))
valtmp = (void *)PMCNULL;
else
if (deep)
- valtmp = (void *)VTABLE_clone(interp, (PMC*)b->value);
+ valtmp = (void *)VTABLE_clone(interp,
(PMC*)_bucket->value);
else
- valtmp = b->value;
+ valtmp = _bucket->value;
break;
default:
@@ -1506,10 +1458,9 @@
Parrot_ex_throw_from_c_args(interp, NULL, -1,
"hash corruption: type = %d\n", hash->entry_type);
};
-
if (key)
parrot_hash_put(interp, dest, key, valtmp);
- }
+ );
}
/*
Index: src/pmc/lexinfo.pmc
===================================================================
--- src/pmc/lexinfo.pmc (revision 48465)
+++ 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/callcontext.pmc
===================================================================
--- src/pmc/callcontext.pmc (revision 48465)
+++ 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 48465)
+++ 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 48465)
+++ 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 48465)
+++ 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 _loc; \
+ for (_loc = (_hash)->mask; _loc >= 0; --_loc) { \
+ HashBucket *_bucket = (_hash)->bucket_indices[_loc]; \
+ while (_bucket) { \
+ _code \
+ _bucket = _bucket->next; \
+ } \
+ } \
+}
+
+#define parrot_hash_iterator(_hash,_bucket,_loc) \
+{ \
+ /* Try to advance current bucket */ \
+ if ((_bucket)) \
+ (_bucket) = (_bucket)->next; \
+ while (!(_bucket)) { \
+ /* If there is no more buckets */ \
+ if ((_loc) == (INTVAL)(_hash)->mask+1) \
+ break; \
+ (_bucket) = (_hash)->bucket_indices[_loc++]; \
+ } \
+}
+
+
typedef void (*value_free)(ARGFREE(void *));
/* To avoid creating OrderedHashItem PMC we reuse FixedPMCArray PMC */
_______________________________________________
http://lists.parrot.org/mailman/listinfo/parrot-dev
