Skip to content

Commit a4091d3

Browse files
stefanbidistefanbidi
authored
Merge pull request #18 from triplef/cfdictionary-tests
Fix hash table logic when removing values.
2 parents 4debac9 + 8b48927 commit a4091d3

File tree

4 files changed

+210
-70
lines changed

4 files changed

+210
-70
lines changed

‎Source/GSHashTable.c

Lines changed: 120 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -113,15 +113,24 @@ GSHashTableSetShouldCount (GSHashTableRef table)
113113

114114

115115

116+
typedef enum
117+
{
118+
_kGSHashTableRetrieve, // retrieval or deletion
119+
_kGSHashTableInsert // insertion only
120+
} _kGSHashTableOperation;
121+
122+
static CFIndex _kGSHashTableBucketCountDeleted = -1;
123+
116124
CF_INLINE void
117125
GSHashTableAddKeyValuePair (GSHashTableRef table,
118126
GSHashTableBucket * bucket, const void *key,
119127
const void *value)
120128
{
121129
GSHashTableRetainCallBack keyRetain = table->_keyCallBacks.retain;
122130
GSHashTableRetainCallBack valueRetain = table->_valueCallBacks.retain;
131+
CFIndex count = bucket->count;
123132

124-
bucket->count++;
133+
bucket->count = (count == _kGSHashTableBucketCountDeleted) ? 1 : (count + 1);
125134
bucket->key = keyRetain ? keyRetain (table->_allocator, key) : key;
126135
bucket->value = valueRetain ? valueRetain (table->_allocator, value) : value;
127136
}
@@ -140,7 +149,8 @@ GSHashTableReplaceKeyValuePair (GSHashTableRef table,
140149
}
141150

142151
CF_INLINE void
143-
GSHashTableRemoveKeyValuePair (GSHashTableRef table, GSHashTableBucket * bucket)
152+
GSHashTableRemoveKeyValuePair (GSHashTableRef table, GSHashTableBucket * bucket,
153+
CFIndex bucketCountDeleted)
144154
{
145155
GSHashTableReleaseCallBack keyRelease = table->_keyCallBacks.release;
146156
GSHashTableReleaseCallBack valueRelease = table->_valueCallBacks.release;
@@ -150,56 +160,75 @@ GSHashTableRemoveKeyValuePair (GSHashTableRef table, GSHashTableBucket * bucket)
150160
if (valueRelease)
151161
valueRelease (table->_allocator, bucket->value);
152162

153-
bucket->count = 0;
163+
bucket->count = bucketCountDeleted;
154164
bucket->key = NULL;
155165
bucket->value = NULL;
156166
}
157167

168+
typedef Boolean (*GSHashTableBucketIsUnoccupiedCallback) (GSHashTableBucket bucket);
169+
170+
CF_INLINE Boolean
171+
GSHashTableBucketIsUnoccupied(GSHashTableBucket bucket, _kGSHashTableOperation operation)
172+
{
173+
switch (operation) {
174+
case _kGSHashTableRetrieve:
175+
return bucket.count == 0; // treat deleted buckets as occupied
176+
case _kGSHashTableInsert:
177+
return bucket.count <= 0; // treat deleted buckets as unoccupied
178+
}
179+
#if __has_builtin(__builtin_unreachable)
180+
__builtin_unreachable();
181+
#else
182+
abort();
183+
#endif
184+
}
185+
186+
static Boolean
187+
GSHashTableEqualPointers (const void *key1, const void *key2)
188+
{
189+
return key1 == key2;
190+
}
191+
158192
static GSHashTableBucket *
159-
GSHashTableFindBucket (GSHashTableRef table, const void *key)
193+
GSHashTableFindBucket (GSHashTableRef table, const void *key,
194+
_kGSHashTableOperation operation)
160195
{
161196
GSHashTableBucket *buckets;
162197
CFIndex capacity;
163-
CFIndex idx;
198+
CFIndex initialIdx, idx;
164199
CFHashCode hash;
165200
Boolean matched;
166201
GSHashTableHashCallBack fHash = table->_keyCallBacks.hash;
167202
GSHashTableEqualCallBack fEqual = table->_keyCallBacks.equal;
203+
204+
if (!fEqual)
205+
fEqual = GSHashTableEqualPointers;
168206

169207
buckets = table->_buckets;
170208
capacity = table->_capacity;
171209
hash = fHash ? fHash (key) : GSHashPointer (key);
172-
idx = hash % capacity;
173-
matched = buckets[idx].key == NULL || (fEqual ?
174-
fEqual (key,
175-
buckets[idx].key) : key ==
176-
buckets[idx].key);
210+
initialIdx = idx = hash % capacity;
211+
matched = GSHashTableBucketIsUnoccupied(buckets[idx], operation)
212+
|| (buckets[idx].key && fEqual (key, buckets[idx].key));
177213

178214
if (!matched)
179215
{
180216
CFHashCode hash2 = 1 + ((hash / capacity) % (capacity - 1));
181-
182-
if (fEqual)
183-
{
184-
do
185-
{
186-
hash += hash2;
187-
idx = hash % capacity;
188-
}
189-
while (buckets[idx].key != NULL && !fEqual (key, buckets[idx].key));
190-
}
191-
else
217+
218+
do
192219
{
193-
do
194-
{
195-
hash += hash2;
196-
idx = hash % capacity;
197-
}
198-
while (buckets[idx].key != NULL && key != buckets[idx].key);
220+
hash += hash2;
221+
idx = hash % capacity;
222+
matched = GSHashTableBucketIsUnoccupied(buckets[idx], operation)
223+
|| (buckets[idx].key && fEqual (key, buckets[idx].key));
199224
}
225+
while (!matched && idx != initialIdx);
200226
}
201227

202-
return &buckets[idx];
228+
// match should be NULL for _kGSHashTableRetrieve ONLY
229+
assert(matched || operation == _kGSHashTableRetrieve);
230+
231+
return matched ? &buckets[idx] : NULL;
203232
}
204233

205234

@@ -275,7 +304,8 @@ GSHashTableCreate (CFAllocatorRef alloc, CFTypeID typeID,
275304
{
276305
for (idx = 0; idx < numValues; ++idx)
277306
{
278-
bucket = GSHashTableFindBucket (new, keys[idx]);
307+
bucket = GSHashTableFindBucket (new, keys[idx],
308+
_kGSHashTableInsert);
279309
GSHashTableAddKeyValuePair (new, bucket, keys[idx], values[idx]);
280310
new->_count += 1;
281311
}
@@ -304,7 +334,8 @@ GSHashTableCreateCopy (CFAllocatorRef alloc, GSHashTableRef table)
304334
{
305335
if (buckets[idx].key)
306336
{
307-
bucket = GSHashTableFindBucket (new, buckets[idx].key);
337+
bucket = GSHashTableFindBucket (new, buckets[idx].key,
338+
_kGSHashTableInsert);
308339
GSHashTableAddKeyValuePair (new, bucket, buckets[idx].key,
309340
buckets[idx].value);
310341
new->_count += 1;
@@ -338,16 +369,22 @@ GSHashTableEqual (GSHashTableRef table1, GSHashTableRef table2)
338369
end = current + table1->_capacity;
339370
keyEqual = table1->_keyCallBacks.equal;
340371
valueEqual = table1->_valueCallBacks.equal;
372+
373+
if (!keyEqual)
374+
keyEqual = GSHashTableEqualPointers;
375+
if (!valueEqual)
376+
valueEqual = GSHashTableEqualPointers;
377+
341378
while (current < end)
342379
{
343380
if (current->count > 0)
344381
{
345-
other = GSHashTableFindBucket (table2, current->key);
346-
if (current->count != other->count
347-
|| keyEqual ? !keyEqual (current->key, other->key) :
348-
current->key != other->key
349-
|| valueEqual ? !valueEqual (current->value, other->value) :
350-
current->value != other->value)
382+
other = GSHashTableFindBucket (table2, current->key,
383+
_kGSHashTableRetrieve);
384+
if (!other
385+
|| current->count != other->count
386+
|| !keyEqual (current->key, other->key)
387+
|| !valueEqual (current->value, other->value))
351388
return false;
352389
}
353390
++current;
@@ -369,9 +406,9 @@ Boolean
369406
GSHashTableContainsKey (GSHashTableRef table, const void *key)
370407
{
371408
GSHashTableBucket *bucket;
372-
bucket = GSHashTableFindBucket (table, key);
409+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
373410

374-
return bucket->count > 0 ? true : false;
411+
return bucket ? (bucket->count > 0 ? true : false) : false;
375412
}
376413

377414
Boolean
@@ -381,12 +418,14 @@ GSHashTableContainsValue (GSHashTableRef table, const void *value)
381418
GSHashTableBucket *buckets = table->_buckets;
382419
GSHashTableEqualCallBack equal = table->_valueCallBacks.equal;
383420

421+
if (!equal)
422+
equal = GSHashTableEqualPointers;
423+
384424
for (idx = 0; idx < table->_capacity; ++idx)
385425
{
386426
if (buckets[idx].key)
387427
{
388-
if (equal ? equal (value, buckets[idx].value) :
389-
value == buckets[idx].value)
428+
if (equal (value, buckets[idx].value))
390429
return true;
391430
}
392431
}
@@ -402,8 +441,10 @@ GSHashTableGetCount (GSHashTableRef table)
402441
CFIndex
403442
GSHashTableGetCountOfKey (GSHashTableRef table, const void *key)
404443
{
405-
GSHashTableBucket *bucket = GSHashTableFindBucket (table, key);
406-
return bucket->count;;
444+
GSHashTableBucket *bucket;
445+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
446+
447+
return bucket ? bucket->count : 0;
407448
}
408449

409450
CFIndex
@@ -414,12 +455,14 @@ GSHashTableGetCountOfValue (GSHashTableRef table, const void *value)
414455
GSHashTableBucket *buckets = table->_buckets;
415456
GSHashTableEqualCallBack equal = table->_valueCallBacks.equal;
416457

458+
if (!equal)
459+
equal = GSHashTableEqualPointers;
460+
417461
for (idx = 0; idx < table->_capacity; ++idx)
418462
{
419463
if (buckets[idx].key)
420464
{
421-
if (equal ? equal (value, buckets[idx].value) :
422-
value == buckets[idx].value)
465+
if (equal (value, buckets[idx].value))
423466
count += buckets[idx].count;
424467
}
425468
}
@@ -451,9 +494,9 @@ const void *
451494
GSHashTableGetValue (GSHashTableRef table, const void *key)
452495
{
453496
GSHashTableBucket *bucket;
454-
bucket = GSHashTableFindBucket (table, key);
455-
456-
return bucket->value;
497+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
498+
499+
return bucket ? bucket->value : NULL;
457500
}
458501

459502

@@ -478,7 +521,8 @@ GSHashTableRehash (GSHashTableRef table, CFIndex newCapacity)
478521
{
479522
if (oldBuckets[idx].key)
480523
{
481-
bucket = GSHashTableFindBucket (table, oldBuckets[idx].key);
524+
bucket = GSHashTableFindBucket (table, oldBuckets[idx].key,
525+
_kGSHashTableInsert);
482526
GSHashTableAddKeyValuePair (table, bucket, oldBuckets[idx].key,
483527
oldBuckets[idx].value);
484528
}
@@ -562,7 +606,8 @@ GSHashTableCreateMutableCopy (CFAllocatorRef alloc, GSHashTableRef table,
562606
{
563607
if (buckets[idx].key)
564608
{
565-
bucket = GSHashTableFindBucket (new, buckets[idx].key);
609+
bucket = GSHashTableFindBucket (new, buckets[idx].key,
610+
_kGSHashTableInsert);
566611
GSHashTableAddKeyValuePair (new, bucket, buckets[idx].key,
567612
buckets[idx].value);
568613
new->_count += 1;
@@ -580,8 +625,11 @@ GSHashTableAddValue (GSHashTableRef table, const void *key, const void *value)
580625

581626
GSHashTableGrowIfNeeded (table);
582627

583-
bucket = GSHashTableFindBucket (table, key);
584-
if (bucket->count == 0)
628+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
629+
if (!bucket)
630+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableInsert);
631+
632+
if (bucket->count <= 0)
585633
{
586634
GSHashTableAddKeyValuePair (table, bucket, key, value);
587635
table->_count += 1;
@@ -594,8 +642,8 @@ GSHashTableReplaceValue (GSHashTableRef table, const void *key,
594642
{
595643
GSHashTableBucket *bucket;
596644

597-
bucket = GSHashTableFindBucket (table, key);
598-
if (bucket->count > 0)
645+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
646+
if (bucket && bucket->count > 0)
599647
GSHashTableReplaceKeyValuePair (table, bucket, key, value);
600648
}
601649

@@ -606,7 +654,10 @@ GSHashTableSetValue (GSHashTableRef table, const void *key, const void *value)
606654

607655
GSHashTableGrowIfNeeded (table);
608656

609-
bucket = GSHashTableFindBucket (table, key);
657+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
658+
if (!bucket)
659+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableInsert);
660+
610661
if (bucket->count > 0)
611662
{
612663
GSHashTableReplaceKeyValuePair (table, bucket, key, value);
@@ -627,7 +678,7 @@ GSHashTableRemoveAll (GSHashTableRef table)
627678
for (idx = 0; idx < table->_capacity; ++idx)
628679
{
629680
if (buckets[idx].count > 0)
630-
GSHashTableRemoveKeyValuePair (table, &buckets[idx]);
681+
GSHashTableRemoveKeyValuePair (table, &buckets[idx], 0);
631682
}
632683
table->_count = 0;
633684
}
@@ -637,16 +688,20 @@ GSHashTableRemoveValue (GSHashTableRef table, const void *key)
637688
{
638689
GSHashTableBucket *bucket;
639690

640-
GSHashTableShrinkIfNeeded (table);
641-
642-
bucket = GSHashTableFindBucket (table, key);
643-
if (bucket->count > 1)
644-
{
645-
bucket->count -= 1;
646-
}
647-
else if (bucket->count == 1)
691+
bucket = GSHashTableFindBucket (table, key, _kGSHashTableRetrieve);
692+
if (bucket)
648693
{
649-
GSHashTableRemoveKeyValuePair (table, bucket);
650-
table->_count -= 1;
694+
if (bucket->count > 1)
695+
{
696+
bucket->count -= 1;
697+
}
698+
else if (bucket->count == 1)
699+
{
700+
GSHashTableRemoveKeyValuePair (table, bucket,
701+
_kGSHashTableBucketCountDeleted);
702+
table->_count -= 1;
703+
}
704+
705+
GSHashTableShrinkIfNeeded (table);
651706
}
652707
}

‎Tests/CFArray/bridge.m

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ void testNSonCF(void)
2929
PASS_CF([nsarray count] == count,
3030
"-count works on a CFArray");
3131

32-
[nsarray addObject: CFSTR("5")];
32+
[nsarray addObject: (id)CFSTR("5")];
3333

3434
PASS_CF([nsarray count] == count+1,
3535
"-addObject: adds a value into a CFArray");

0 commit comments

Comments
 (0)