some functions now returning `const void *`

[scossu]

I noticed that in 84d0d3b#diff-9257c021257b8c2773c58e19016dbd9499f0524100e98dd4616f8a9a55e8b13fR33 return types for hashmap_get, hashmap_set, hashmap_delete, and hashmap_probe were changed to constant. This is raising warnings on code that I have written previously.

Before I change the code on my end, I would like to understand the rationale behind this change. If the intent is to discourage changing the underlying bucket data, I understand the change in hashmap_get and hashmap_probe. For hashmap_set and hashmap_delete, however, I don't see the point in keeping the return values constant, since they are no longer managed by the hashmap. Can you explain? Thanks.

[scossu]

Looking at the code, I noticed that both hashmap_set and hashmap_delete return map->spare which is a copy of the hash data. I don't see that being freed by hashmap_free. Is that supposed to be handed off to the user, who is in charge of freeing it?

[tidwall]

The spare and exdata are two special buckets that are part of the same allocation as the other fields in the hashmap.

hashmap.c/hashmap.c

Line 129 in 1c13992

size_t size = sizeof(struct hashmap)+bucketsz*2;

They both are freed at the same time that the hashmap structure is freed.

Regarding the const void *, that can be change back for compatibility. Though for a hashmap_get operation the returned pointer does directly reference memory in the hashmap bucket.

The const void * was added a while ago as a safe guard to help me remember not to hang on to the returned pointer. If I remember correctly I ran into an alignment issue at one point when using weird element sizes.

I now use this pattern in general.

struct item item = { 0 }; 
const void *ptr = hashmap_get(map, key);
if (ptr) {
        memcpy(&item, ptr, sizeof(struct item));
}

I wish I would have originaly create the methods like:

struct item item = { 0 };
bool found = hashmap_get(map, key, &item);
if (found) {
        // item found
} else {
        // item not found
}

[scossu]

I think adding const to the get and scan functions is good, as the hash function could use any part of the item (even separately allocated members), so it seems that changing the bucket data directly could cause the hash to no longer match the data.

I caught myself doing awkward things with retrieved bucket data, so it's good to be explicit about having to copy the data in order to manipulate it. E.g. to add or replace an element, instead of doing a hashmap_get and manipulating the returned data, I can do a hashmap_delete, copy the data (if found), replace the parts I want modified, and do a hashmap_set.

Regarding the spare element returned by hashmap_delete and hashmap_set, I understand that your suggestion is to also copy that, because if I use it I won't mess up the hashmap, but the structure could be freed while I am still using it if I free the hash map. Correct?

[tidwall]

That's correct. Holding on to the returned pointer is always a risk. I found it to be good practice to always copy the contents immediately.
Even with a ptr = hashmap_get(map, key). That pointer's contents could get freed on the next hashmap_set or hashmap_delete. The spare pointer will always be safe for the life of the hashmap, but its contents won't.