1. Would you like to add more error handling for return values from functions like the following?
   • pthread_mutex_lock ⇒ list_sort
   • pthread_spin_init ⇒ ht_set_list
2. How do you think about to annotate any functions with "__attribute__((warn_unused_result))"?

Some quick initial review of libhl turned up some issues with it.

• The use of __USE_UNIX98 is a non-standard feature-test macro which should be replaced with _POSIX_C_SOURCE.
• There are a lot of identifiers that begin with double underscore, for instance __foo. These identifiers are reserved by the implementation (compiler and libc.) perhaps using an hl_ prefix would be better. This is also seen in headers a lot too; __BINOMIAL_HEAP_H__ for instance.
• Failure to use extern "C" in headers for __cplusplus This makes it hard to actually use the library for C++ as the symbol names can be mangled, thus not finding them at link stage.
• #pragma pack(push, ...) is seen a lot. I'm not sure why you're packing these structures, for doing so can only lead to access violations on systems where the read/write may be unaligned and therefor illegal (PPC for instance.)
• Checking for NULL when calling free. This is just redundant as free is required to do this check. Doing this only leads to false-positives when running the code through static-analysis tools like coverity.
• Linked list is not thread-safe. get_entry_position needs to hold a lock for a new list node can be inserted into the list in another thread causing the position to be wrong. This can cause a whole slew of problems.
• Failure to check return value of malloc. It can fail, without checking and handling this case, this library is not usable for robust systems.

I would like to point out that identifiers like "__tagged_value" and "_HL_HASHTABLE_H_" do eventually not fit to the expected naming convention of the C language standard. Would you like to adjust your selection for unique names?

Currently the tagged value structure for linked lists assumes that the length of a value can fit inside uint32_t. While I'd agree that value types larger than 0xffffffff bytes are pathological; use of size_t instead is of no harm and does not blindly break the interface contract of strlen and would allow you to remove the cast on this line in particular newval->vlen = (uint32_t)strlen((char *)val);.

When representing the size of something it's always best to use size_t which is specifically guaranteed to be large enough to hold the size of any represent-able allocation or object in memory. This is why allocation functions expect size_t, why strlen returns it and why the sizeof operator does as well.

Hi,

I know that this patch is a bit irritating and it will not make any difference at all. But I wanted to contribute to such a cool software and found low hanging fruit :-)

Note that I changed op in ATOMIC_READ to OR.

Also, it's quite possible that I'm stupid and you wrote this for very good and very smart reasons :-) If so, just disregard me (but explain, please :-))

P.S. return value of REFCNT_ATOMIC_INCREMENT/DECREMENT is never checked

hi, author. I use create 5 thread to pop queue's arg, there are 10000 args that push to queue. But sometimes, the process will be crash. And from the crash log, i see that queue will call refcnt free and then process crash.

Hello, I am looking for a hashtable which can hold millions or even more key-value pairs. Can this hashtable hold such payload, and give a good performance when there is some rebalance?

The macro "AC_INIT" supports that the needed version information is determined by specific macro calls before. Would you like to reuse anything from the current autoconf approach for your build specification?

I suggest to reuse a higher level build system than your current small make file so that powerful checks for software features will become easier.

• CMake
• Autotools

I'm currently unable to share the code that triggers this memory leak but I have traced it using valgrind through the following calls:

==77605== 4,811,032 (4,066,576 direct, 744,456 indirect) bytes in 187,682 blocks are definitely lost in loss record 10 of 10
==77605==    at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==77605==    by 0x10E6F3: binomial_tree_merge (binheap.c:399)
==77605==    by 0x10E8B3: binheap_insert (binheap.c:427)
==77605==    by 0x10D12E: pqueue_insert (pqueue.c:78)

This is one of ten leaks that valgrind detects over a relatively short span of inserting items into a priority queue. It seems like the realloc from binomial_tree_merge is not beeing freed.

I am doing something funky in that I am passing size_t values and casting them as void* types in my priority queue but this seems to work. I don't know if there is an underlying assumption anywhere that the values must be pointers (for example, this approach doesn't work with hastable.c because the hash function accesses the pointers; I don't believe pqueue or binheap do this, though).

I suggest to [reuse the software "libtool"](https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Libtool.htm Libtool usage"). Would you like to add the call of the macro "LT_INIT"" to your build specification?

The number of successfully read from rqueue sometimes less than the number of successfully write to rqueue.

Here is the test code. https://gist.github.com/mingpepe/9d6ec5f17bdb60094d7fc980df3437ce

But i am not found the bug yet.