Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

copy out keys also in HashMap.copy_keys #1569

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

hwchen
Copy link
Contributor

@hwchen hwchen commented Oct 24, 2024

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
Copy link
Contributor Author

hwchen commented Oct 24, 2024

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.

@lerno
Copy link
Collaborator

lerno commented Oct 25, 2024

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
Copy link
Collaborator

lerno commented Oct 25, 2024

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

@hwchen
Copy link
Contributor Author

hwchen commented Oct 25, 2024

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.

@lerno
Copy link
Collaborator

lerno commented Oct 25, 2024

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 lerno merged commit fd1898b into c3lang:master Oct 25, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants