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

[#24] Add union and unionWith #29

Merged
merged 1 commit into from
Jul 9, 2018
Merged

[#24] Add union and unionWith #29

merged 1 commit into from
Jul 9, 2018

Conversation

chshersh
Copy link
Contributor

@chshersh chshersh commented Jul 9, 2018

Resolves #24

Unfortunately, implementation uses containers package so it won't be to easy to remove containers from dependencies... It's possible, but not trivial. But looks like at least we don't need vector after PR #28 by @vrom911.

@chshersh chshersh added this to the v1.0 Hackage upload milestone Jul 9, 2018
@chshersh chshersh self-assigned this Jul 9, 2018
@int-index
Copy link
Collaborator

int-index commented Jul 9, 2018

OK, let's keep using containers for now. At some point in the future let's try to rewrite this to something more efficient:

  1. Allocate new mutable vectors of length n+m.
  2. Traverse input vectors simultaneously, copying elements in the right order. We should be able to take advantage of the fact that the elements are in a particular order. For regular sorted vectors it's very simple, for our cache-friendly order it can be tricky, but should be possible (I hope)
  3. While doing this, count the actual amount of elements copied (it can be less than n+m due to conflicts). Shrink the mutable vector.
  4. Freeze and return.

@chshersh
Copy link
Contributor Author

chshersh commented Jul 9, 2018

@int-index Thanks for feedback! I've created issue #30 using your comment.

@vrom911 vrom911 merged commit 979991b into master Jul 9, 2018
@vrom911 vrom911 deleted the chshersh/24-union branch July 9, 2018 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants