-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat: add Set stdlib module #466
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thanks for putting this together 😄
Re: make
vs. init
, make
creates an empty structure, and init
allows you to create a structure with some initial data. Hopefully that answers your question, but let me know if that's not what you were asking.
I think this largely looks good, but for the backing data structure, I believe that we do want it to be an Array, like Maps. Sets are essentially just Map<t, Void>
, so the implementation can be pretty much exactly the same as Map, but the buckets would not have a value
property. This way, your Set.contains
and Set.add
are both O(1) in the average case, versus O(n) to search/insert into a list.
Let me know if that makes sense or if there's anything I can clarify!
d3381c1
to
e4d47c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is AWESOME! A few questions/changes that might be able to improve it.
stdlib/set.gr
Outdated
let set = make(); | ||
List.forEach((key) => { | ||
add(key, set); | ||
}, List.unique(list)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to drop the unique
call here, since add
should take care of it (and it'll be faster!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, add
should be enough. Thank you.
stdlib/set.gr
Outdated
let set = make(); | ||
Array.forEach((key) => { | ||
add(key, set); | ||
}, Array.unique(array)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Awesome work @zweimach!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! So excited to have this done! 🎉
Add initial implementation of Set module using
List as thethe same underlying data structure as Map.It reuses many modified helper functions from the Map module.
Maybe we could unify those functions?
@ospencer, what is the convention for
init
andmake
family of functions?