xant / libhl

Simple and fast C library implementing a thread-safe API to manage hash-tables, linked lists, lock-free ring buffers and queues
GNU Lesser General Public License v3.0
420 stars 117 forks source link

Consider using size_t to indicate size for tagged values in linked list #11

Closed graphitemaster closed 9 years ago

graphitemaster commented 9 years ago

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.

graphitemaster commented 9 years ago

Querying the size of elements in most of these containers all return uint32_t as well. Making them use size_t instead. As well as for indices would make this code much more scale-able for different architectures and more correct.

xant commented 9 years ago

ok for the vlen member in the tagged_value_t to be size_t ... but I think that changing the whole api in all the containers to return 'their capacity' as size_t instead of uint32_t seems a bit overkill and not necessary.

Anyway, apart for tagged values in linkedlists, where do you see other size members being uint32_t ? I see that all the other containers already use size_t for size members ... linkedlist was written long time ago and probably wasn't fully updated when imported in libhl.

The only thing I see being uint32_t across all the containers is the 'number of elements that will fit in the container'

xant commented 9 years ago

6dbe9834112d9ba08b271dc08bc4a13919028476

xant commented 9 years ago

actually , after giving a closer look, changing the capacity of all containers (and hence the position indices for linkedlist) and the relative API to use size_t instead of uint32_t seems pretty trivial. I might consider such a change at some point.