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

Add a Set type matching the std HashSet API #17

Closed
cuviper opened this issue Jan 23, 2020 · 11 comments
Closed

Add a Set type matching the std HashSet API #17

cuviper opened this issue Jan 23, 2020 · 11 comments

Comments

@cuviper
Copy link
Collaborator

cuviper commented Jan 23, 2020

Just like in std, it can start with the simple definition wrapping a map:

pub struct FlurryHashSet<T, S = RandomState> {
    map: FlurryHashMap<T, (), S>,
}
@jonhoo
Copy link
Owner

jonhoo commented Jan 23, 2020

The Java code's KeySetView may also be of interest.

@jonhoo
Copy link
Owner

jonhoo commented Mar 17, 2020

We now have a basic HashSet type, but it'd be great to get something along the lines of HashMapRef for HashSet too!

@GarkGarcia
Copy link
Contributor

GarkGarcia commented Mar 22, 2020

We now have a basic HashSet type, but it'd be great to get something along the lines of HashMapRef for HashSet too!

Is that a case of simply wrapping HashMapRef<T, ()> in a newtype? I could work on that if that's the case.

@twe4ked
Copy link
Contributor

twe4ked commented Mar 23, 2020

I think you'd want to wrap HashSet with HashSetRef similar to how HashMap is wrapped by HashMapRef.

@GarkGarcia
Copy link
Contributor

I think you'd want to wrap HashSet with HashSetRef similar to how HashMap is wrapped by HashMapRef.

That's interesting. I'm not quite sure which way would be the most efficient memory wise. @jonhoo @cuviper opinions?

@GarkGarcia
Copy link
Contributor

GarkGarcia commented Mar 24, 2020

We'd be wrapping a wrapper either way...
I think wrapping HashMap<T, ()> directly should make the type's layout flatter, but I'm not sure.

@jonhoo
Copy link
Owner

jonhoo commented Mar 24, 2020

Ah, no, I was imagining we just do the same trick with HashSetRef as with HashMapRef, not that we wrap HashMapRef to make HashSetRef!

@GarkGarcia
Copy link
Contributor

Ah, no, I was imagining we just do the same trick with HashSetRef as with HashMapRef, not that we wrap HashMapRef to make HashSetRef!

Makes sense.

@GarkGarcia
Copy link
Contributor

@jonhoo What about the wrapping HashMap<T, ()> directly proposal?

@jonhoo
Copy link
Owner

jonhoo commented Mar 25, 2020

I think we should wrap HashSet, because HashSet actually has different methods than HashMap, and having the wrapper also do the translation from set-like methods to map-like methods (which HashSet is already doing) seems like unfortunate duplication.

jonhoo added a commit that referenced this issue Mar 27, 2020
@jonhoo
Copy link
Owner

jonhoo commented Mar 27, 2020

Closed by #78!

@jonhoo jonhoo closed this as completed Mar 27, 2020
jonhoo pushed a commit that referenced this issue Feb 3, 2024
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

No branches or pull requests

4 participants