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

Document and export Base.in! #51636

Merged
merged 5 commits into from
Nov 25, 2023
Merged

Document and export Base.in! #51636

merged 5 commits into from
Nov 25, 2023

Conversation

jakobnissen
Copy link
Contributor

I think in! is a useful general function for users, and would be good to have as official API. Its semantics is clear and unambiguous, while providing a clear performance advantage over the naive implementation.

For more evidence that this functionality is useful, consider:

  • Rust's HashSet::insert works just like this implementation of in!
  • This function was already used in the implementation of Base.unique, precisely for the performance over the naive approach

This PR currently doesn't have any tests - I will add them if the API addition of PR is approved.

@jakobnissen jakobnissen added needs tests Unit tests are required for this change collections Data structures holding multiple items, e.g. sets feature Indicates new feature / enhancement requests labels Oct 8, 2023
@BioTurboNick
Copy link
Contributor

Great find - The name feels incomplete for what it does, but given operations on containers generally return the container, having something different makes sense.

Unless there should (also?) be a method should return a tuple of the set and whether the value was already present or not. pushin!?

No strong opinions here, just some thoughts. I'd like having the operation available.

@petvana
Copy link
Member

petvana commented Oct 8, 2023

Comes from #45156 with some initial discussion.

@jakobnissen jakobnissen removed the needs tests Unit tests are required for this change label Oct 9, 2023
base/set.jl Show resolved Hide resolved
jakobnissen and others added 2 commits October 9, 2023 09:02
Let's hope it makes it into 1.11 !

Co-authored-by: Petr Vana <petvana@centrum.cz>
@jakobnissen jakobnissen added the awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Oct 26, 2023
@vtjnash vtjnash merged commit 93d4740 into JuliaLang:master Nov 25, 2023
7 checks passed
mkitti pushed a commit to mkitti/julia that referenced this pull request Dec 9, 2023
I think `in!` is a useful general function for users, and would be good
to have as official API. Its semantics is clear and unambiguous, while
providing a clear performance advantage over the naive implementation.

For more evidence that this functionality is useful, consider:
* Rust's `HashSet::insert` works just like this implementation of `in!`
* This function was already used in the implementation of `Base.unique`,
precisely for the performance over the naive approach

Comes from JuliaLang#45156 with some initial discussion.
@inkydragon inkydragon removed the awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Feb 20, 2024
@jakobnissen jakobnissen deleted the in_bang branch April 11, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants