Skip to main content
Code Review

Return to Revisions

2 of 2
edited for tone
200_success
  • 145.5k
  • 22
  • 190
  • 479

hashmap_insert does not correctly implement a hash map:

if(hashmap_node != NULL && hashmap->node_list[hash]->hash_index == key)
{
 return;
}
else
{
 hashmap_node = hashmap_new_node(hash, value);
 hashmap->element_count++;
}

In a hash map, when encountering the same key hash twice, you don't override the old key. Instead you apply another round of the hash function, to guess the next possible location, until you find an empty spot in the hash map.

The same goes when searching elements in a hash map. If the first round doesn't match the key you were searching for, you have to continue iterating until you either find a gap (indicating that the key is not part of the hashmap), or you actually find the specified key.

This also works differently when deleting from a hash map. As the existence of a gap is an indicator that a value has never existed, you can't just delete the first node the hash points to.


HashmapNode ** node_list;

Why fiddle around with sparse allocation? There is no need for that double indirection.

One of the characteristics of self resizing a hash map is, that you only have little overallocation anyway. So just allocate all instances of HashmapNode in continuous memory.


HashmapNode* hashmap_new_node(unsigned int hash_index, int * values);
HashmapNode* hashmap_new_node(unsigned int hash_index,int values_size){}

The compiler should have warned you of the inconsistent function signatures. Be sure to enable and heed your compiler warnings.


hashmap->node_list[i] = malloc(sizeof(HashmapNode));
hashmap->node_list[i] = NULL;

That's a massive memory leak you got there, allocating memory and throwing the reference away. You should use essential tools such as Valgrind to check your code for memory leaks such as this.

And that is not your only memory leak — every second function of yours has one or more.


unsigned int hashmap_hash_func(HashMap *hashmap, unsigned int key)
{
 int hash = key;
 hash = (hash >> 3) * 2654435761;
 hash = hash % hashmap->map_size;
 return hash;
}

You should read Knuth's Multiplicative Hash to understand some very specific requirements of hash functions. Notably, the size of the hash table must be prime. Also, you discarded the most important 3 bits without any good reason.


To be honest? I recommend reimplementing it from scratch after reading more about how a hash map should work.

Ext3h
  • 2.8k
  • 13
  • 17
default

AltStyle によって変換されたページ (->オリジナル) /