c3c icon indicating copy to clipboard operation
c3c copied to clipboard

copy out keys also in HashMap.copy_keys

Open hwchen opened this issue 1 year ago • 1 comments

Copyable keys are copied in when adding entries to HashMap, but not when copying out keys. Now copies keys out as well when Key is COPY_KEYS.

hwchen avatar Oct 24 '24 04:10 hwchen

I originally found this bug when copying out keys that were strings. Somehow, I expected that I would manage the memory for String keys, but it turns out that they are copied by the hashmap. I think there are times when I wouldn't want to copy String keys, not sure what the best solution is here.

hwchen avatar Oct 24 '24 04:10 hwchen

Yeah, this is something I've been pondering. I think that you should probably have a non-copying version and a copying and they should be distinctly named even.

lerno avatar Oct 25 '24 08:10 lerno

Can you add a test for it and add a note in the releasenotes?

lerno avatar Oct 25 '24 08:10 lerno

Ok, updated with test and release note.

Took a quick look, it seems like Zig and Odin require the key memory to be managed separately from the map.

If both user-managed and map-managed keys APIs were used within the same HashMap, I'd probably prefer that the map-managed API is more verbose, like set_with_copy_key. But it would feel a bit strange that you could mix-and-match, if both APIs were available.

Overall, echoing some sentiments I read, since I'm using a lower-level language I'm prepared to manage the memory myself.

hwchen avatar Oct 25 '24 21:10 hwchen

There is a problem with non-retained keys, and that is returning hash maps.

So imagine you want to pass a string-value hash map to a user of your library. The user can free the hash but then the user of the library needs to do a second pass to correctly free the keys IF they need to be freed, which is something the API now must explain to the user. And usually they DO need to be freed – and with the right allocator! - but sometimes they're interned constants and should not be.

So this is kind of the problem.

lerno avatar Oct 25 '24 23:10 lerno