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

Tracking: missing compatibility with std::HashMap & std::HashSet #32

Open
virtualritz opened this issue Nov 26, 2023 · 5 comments
Open

Comments

@virtualritz
Copy link
Contributor

virtualritz commented Nov 26, 2023

I just looked into replacing ahash with gxhash elsewhere and ran into HashMap::new() missing.

I reckon there may be more. I'll have a look later and may add some PRs to this issue. I'd keep it open until I'm certain the gxhash containers are full drop-in replacements for the std versions.

@ogxd ogxd assigned ogxd and unassigned ogxd Nov 26, 2023
@ogxd
Copy link
Owner

ogxd commented Nov 26, 2023

Hi @virtualritz, on my end I'll try in my projects replacing occurrences of HashMaps with it as well :)

@virtualritz
Copy link
Contributor Author

virtualritz commented Nov 26, 2023

It seems the only things missing are new() and with_capacity().
I looked at the aHash crate. They use two traits (orphan rules), HashMapExt and HashSetExt, that provide those and need to be brought into scope if the resp. type alias is being used.

On a sidenote: these aliases and their traits are also behind a std feature as ahash is no_std when the collections are not used.
Maybe GxHash could be no_std too in this case (for embedded etc.)?

Furthermore aHash's type aliases have the same names as their std equivalents (no prefix). I.e. ahash::HashMap is a type alias for std::collections::HashMap and requires ahash::HashMapExt to provide the constructors (to be 100% drop-in).

They also have a newtype wrapper, AHashMap, which does not need additional traits but requires a lot of boilerplate. Maybe something for the future (and should have its own issue).

I suggest to rename the type alias GxHashMap to HashMap and put it behind a std feature, i.e. mirror what ahash is doing.
Then, if anyone asks for it in the future, the newtypes (called GxHashMap and GxHashSet) could be added at a later stage.

That way replacing aHash with GxHash in some third party crate is really just changing the top-level import prefix and the resp. crate in Cargo.toml from ahash to gxhash:

use ahash::{HashMap, HashMapExt};

becomes:

use gxhash::{HashMap, HashMapExt};

Does that make sense? I could have a PR ready in the next hour. I'm on a train home and bored. :]

@ogxd
Copy link
Owner

ogxd commented Nov 26, 2023

I checked in a few projects and I came up to the same conclusion (missing new and with_capacity because those are actually defined only for the HashMap with RandomState).
What you propose sounds great! Very convenient to use. Making it no_std compatible might take some time however, maybe it can be done in two PRs, as you wish!

I'd say std feature should be a default feature (same as in ahash)

@virtualritz
Copy link
Contributor Author

I'm looking into both (the changes and the no_std stuff). Should have something tomorrow.

@virtualritz
Copy link
Contributor Author

There is a PR (#35) that should close this issue.

Unless you wanto to copy aHash and add full newtype wrappers (GxHashMap/GxHashSet) at some point.

@ogxd ogxd linked a pull request Dec 25, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants