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

Allow serializing HashMaps that use a custom allocator #449

Merged
merged 2 commits into from Aug 22, 2023

Conversation

tomcc-reactor
Copy link

@tomcc-reactor tomcc-reactor commented Jul 29, 2023

Hi,
I'm using HashMap with custom allocators (eg. not Global) in my project, and I got hit with a "HashMap doesn't implement trait Serialize" error.

It looks like it has a very trivial fix, eg. exposing the A generic parameter on the impl so that it doesn't default to Global.

I have a very limited understanding of HashBrown so let me know if this is a change that makes sense!

  • Is crate::raw::Allocator the right way to import the Allocator trait?
  • Do you need Deserialize to be fixed as well?
  • What about HashSet?

Anyway, if the change makes sense I can try to clean it up a bit more :)

@Amanieu
Copy link
Member

Amanieu commented Jul 30, 2023

  • Is crate::raw::Allocator the right way to import the Allocator trait?

Yes, but put it in a use statement.

  • Do you need Deserialize to be fixed as well?

Yes, this should use A: Allocator + Default.

  • What about HashSet?

Please implement this for HashSet as well.

@tomcc-reactor
Copy link
Author

Done, let me know if it looks ok.
It's just a bit sad that serde doesn't really allow a non-default allocator, but for now that works for my use case.
HashSet::deserialize_in_place doesn't require default in practice, but the trait bound will prevent it from working...

@Amanieu
Copy link
Member

Amanieu commented Aug 22, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 22, 2023

📌 Commit 065b6ee has been approved by Amanieu

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 22, 2023

⌛ Testing commit 065b6ee with merge 5e578e7...

@bors
Copy link
Collaborator

bors commented Aug 22, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 5e578e7 to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Aug 22, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 5e578e7 to master...

@bors bors merged commit 5e578e7 into rust-lang:master Aug 22, 2023
25 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.

None yet

3 participants