Return to Answer

(Intending not to go into general pros and cons of hash table implementation.
Listing problems looks a bit like there isn't good (like declaring several parameters pointer to const) -
Consider anything not mentioned
Looking innocent enough, at least.)

• What does specifying a table_size of 7 or 4711 in CGL_hashtable_create() mean? Does that limit the number of entries possible? Does the implementation go to lengths to keep a tolerable collision ratio for that many entries?
  Or as key_size? (Hint: It does not mean that many bytes will be compared.)
  Will key and/or value specified in CGL_hashtable_set() be copied, so the caller is free to subsequently change at least "the key"?
  → Document your code. In the code, preferably.

• struct declarations:
  • old wisdom suggested to declare members in order of decreasing size to minimise alignment overhead.
  • C99 allows the last member to be an array of unknown size.
  This allows for keys not limited by any compile-time #define at some additional hassle.
  (Starting with table->entries no longer being an array of structs, but of pointers.
  Which may have general pros.)

• __CGL_hashtable_entry_destroy():
  • Double underscore names are reserved for "standard" library use - I forget the details.
  • The NULL-checks are redundant: Consider getting rid of one of them.
  • The destroy flag is dispensable: You can check entry to be inside table->entries.
  • The paranoid set pointers to NULL or garbage after specifying them in free().

• __CGL_hashtable_get_key_size_and_table_index():
  Using strlen((const char*)key) is one too few:
  Prefixes match longer strings.

• __CGL_hashtable_get_entry_ptr():
  Doesn't check current_entry.set.

• CGL_hashtable_create():
  Doesn't check allocation results.
  Far-out parameter values go silent.

• CGL_hashtable_set():
  • Just overwrites entry_ptr->value ... -
  this probably is the leak valgrind reported (there may be more).
  • ... in duplicated code: avoidably hard to maintain.
  • Doesn't check allocation results.
  • I don't like "pre-filling" a struct and copying it once you know the destination.

• CGL_hashtable_remove():
  Doesn't update entry->next_entry->prev_entry.
  This spells trouble when removing the formerly next entry, too.

• Usage:
  Never checks the result of CGL_hashtable_get(): Keys not found go unnoticed.

(Intending not to go into general pros and cons of hash table implementation.
Listing problems looks a bit like there isn't good (like declaring several parameters pointer to const) -
Consider anything not mentioned
Looking innocent enough, at least.)

• What does specifying a table_size of 7 or 4711 in CGL_hashtable_create() mean? Does that limit the number of entries possible? Does the implementation go to lengths to keep a tolerable collision ratio for that many entries?
  Or as key_size? (Hint: It does not mean that many bytes will be compared.)
  Will key and/or value specified in CGL_hashtable_set() be copied, so the caller is free to subsequently change at least "the key"?
  → Document your code. In the code, preferably.
  I see an interface problem in CGL_hashtable_get() not getting an output buffer size, inviting overruns.

• struct declarations:
  • old wisdom suggested to declare members in order of decreasing size to minimise alignment overhead.
  • C99 allows the last member to be an array of unknown size.
  This allows for keys not limited by any compile-time #define at some additional hassle.
  (Starting with table->entries no longer being an array of structs, but of pointers.
  Which may have general pros.)

• __CGL_hashtable_entry_destroy():
  • Double underscore names are reserved for "standard" library use - I forget the details.
  • The NULL-checks are redundant: Consider getting rid of one of them.
  • The destroy flag is dispensable: You can check entry to be inside table->entries.
  • The paranoid set pointers to NULL or garbage after specifying them in free().

• __CGL_hashtable_get_key_size_and_table_index():
  Using strlen((const char*)key) is one too few:
  Prefixes match longer strings.

• __CGL_hashtable_get_entry_ptr():
  Doesn't check current_entry.set.

• CGL_hashtable_create():
  Doesn't check allocation results.
  Far-out parameter values go silent.

• CGL_hashtable_set():
  • Just overwrites entry_ptr->value ... -
  this probably is the leak valgrind reported (there may be more).
  • ... in duplicated code: avoidably hard to maintain.
  • Doesn't check allocation results.
  • I don't like "pre-filling" a struct and copying it once you know the destination.

• CGL_hashtable_remove():
  Doesn't update entry->next_entry->prev_entry.
  This spells trouble when removing the formerly next entry, too.

• Usage:

  • Never checks the result of CGL_hashtable_get():
    • Keys not found go unnoticed.
    • doesn't limit the number of characters printed in
    printf("Name...: %s\n", buffer)