-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 hashWangYi1
#13823
Merged
Merged
Add hashWangYi1
#13823
Changes from 25 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
70c81c4
Unwind just the "pseudorandom probing" (whole hash-code-keyed variable
c-blake 0433f4b
Fix `data.len` -> `dataLen` problem.
c-blake 5dc4b0d
Merge /u/cb/pkg/nim/Nim-devel into devel
c-blake 5a43e2c
This is an alternate resolution to https://github.com/nim-lang/Nim/is…
c-blake 1b69485
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake e89fd81
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake 4b0c41a
Re-organize to work around `when nimvm` limitations; Add some tests; Add
c-blake a7cada9
Add less than 64-bit CPU when fork.
c-blake 4ed8e1a
Fix decl instead of call typo.
c-blake 875e7fb
First attempt at fixing range error on 32-bit platforms; Still do the
c-blake dc4dba2
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake b3510c3
A second try at making 32-bit mode CI work.
c-blake 38fe03c
Use a more systematic identifier convention than Wang Yi's code.
c-blake 1474ae4
Fix test that was wrong for as long as `toHashSet` used `rightSize` (a
c-blake b78e18d
Fix another stringified test depending upon hash order.
c-blake 29424cc
Oops - revert the string-keyed test.
c-blake add55fe
Fix another stringify test depending on hash order.
c-blake 00a708e
Add a better than always zero `defined(js)` branch.
c-blake 2febb78
It turns out to be easy to just work all in `BigInt` inside JS and thus
c-blake 33240f9
These string hash tests fail for me locally. Maybe this is what causes
c-blake f5aae61
Oops. That failure was from me manually patching string hash in hashe…
c-blake 56e1df8
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake 131667a
Import more test improvements from https://github.com/nim-lang/Nim/pu…
c-blake fca9bb5
Fix bug where I swapped order when reverting the test. Ack.
c-blake 1f11f9c
Oh, just accept either order like more and more hash tests.
c-blake 0ca8acc
Iterate in the same order.
c-blake 0c6162a
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake 41dd4a6
`return` inside `emit` made us skip `popFrame` causing weird troubles.
c-blake 137e1f1
Oops - do Windows branch also.
c-blake b525246
`nimV1hash` -> multiply-mnemonic, type-scoped `nimIntHash1` (mnemonic
c-blake aa30554
Re-organize `when nimvm` logic to be a strict `when`-`else`.
c-blake fa879a5
Merge other changes.
c-blake 97e685b
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake 67eab12
Lift constants to a common area.
c-blake f3c236e
Fall back to identity hash when `BigInt` is unavailable.
c-blake dfe9ee8
Merge branch 'devel' of https://github.com/Araq/Nim into add_hashWangYi1
c-blake e97c81a
Increase timeout slightly (probably just real-time perturbation of CI
c-blake File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this change still needed? Does the example work without it?
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.
That change is necessary because
toHashSet
also usesrightSize
for efficiency (to prevent doubling-up however many times). If you drop theassert
you could drop therightSize
. If you compare the output sets by membership, not bystring
/$
which depends upon hash order then you could also drop therightSize
. OTOH, if you are considering it a "good example" thenrightSize
is good style since you start with the right size of an output table which is always good style if you know it (and sadly not often provided by hash table libraries).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.
Oh, also it's part of a
block toSeqAndString
which, at least to me, made it seem more correct to keep the stringification but make theHashSet
table size the same, not whatever the default initial size is. "Hash order" is really a concept inherently relative to a given hash table size since it'shash() and mask
. So, if you did want to drop the$
s we would also probably want to change the block title. But I think it's fine as-is.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.
The change to
sugar.nim
is, however, not strictly necessary but the sugar-internal consistency might be considered "better style".