Skip to content

Hash implementation for NSIndexSet #2881

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

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

lxbndr
Copy link
Contributor

@lxbndr lxbndr commented Sep 16, 2020

We noticed that NSIndexSet is missing implementation of hash property. Not a big deal, as NSObject provides default hash, but there are some cons:

  • Equal NSIndexSets would have different hashes based on their object identifier
  • IndexSet is backed by NSIndexSet, deriving same inconvenience
  • This behavior is not consistent with Apple Foundation

After some experiments with NSIndexSet from macOS Foundation, these details were observed:

  • Calculation complexity is O(1). No matter how many indexes/ranges is stored, hash is always fast.
  • The only parameters affecting hash are: count of indexes, first and last index value. This explains constant complexity.
  • Empty index set has hash equal to 0.

I think the most important detail here is calculation speed, because my first thoughts on implementation were about iterating over all ranges in set. Which is, obviously, slower. But we still could take into account number of ranges to make hash better.

Based on all above, we'd like to suggest hash implementation powered by standard Hasher and based on index count, range count, first and last index.

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@lxbndr lxbndr changed the base branch from master to main September 23, 2020 07:41
@millenomi millenomi merged commit 68b260e into swiftlang:main Oct 1, 2020
@lxbndr lxbndr deleted the nsindexset-hash branch October 1, 2020 21:19
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

Successfully merging this pull request may close these issues.

3 participants