Alternate to https://github.com/nim-lang/Nim/pull/15915 #15937
Conversation
resolve the problem mentioned there (`hash() == 0`) as well as to close nim-lang#15624
|
when this merged also merge #15933 for testing against c, cpp , js backend. |
lib/pure/hashes.nim
Outdated
| @@ -114,7 +114,11 @@ proc hashWangYi1*(x: int64|uint64|Hash): Hash {.inline.} = | |||
| const P1 = 0xe7037ed1a0b428db'u64 | |||
| const P58 = 0xeb44accab455d165'u64 xor 8'u64 | |||
| when nimvm: | |||
| cast[Hash](hiXorLo(hiXorLo(P0, uint64(x) xor P1), P58)) | |||
| when defined(js): # NOTE: Nim int64<->Number => JS has limited 32-bit hash. | |||
| result = cast[Hash](hiXorLo(hiXorLo(P0, uint64(x) xor P1), P58)) and | |||
There was a problem hiding this comment.
subexpression cast[Hash](hiXorLo(hiXorLo(P0, uint64(x) xor P1), P58)) repeated 3 times; factor it using some auxiliary template
There was a problem hiding this comment.
Ok. I kept the cast[Hash]() part open coded since it is short and there is one spot where it seems more clear to say cast[Hash](a) and cast[Hash](b) and have the type matching for the binary operator be obvious.
|
It's better to have some tests in this branch. |
no need mention PR nim-lang#15915, fixed in nim-lang#15937
|
test already there , old test just not test agains to js , added in my PR. |
At least these tests should be tested locally before merging this PR. |
{ though this was only a move from 2 copies to 3 copies. ;-) }
|
@xflywind - I had already run locally run the While I can, of course, fold in that { Also, if anyone cares about the limited 32-bit range, I think we can just add 5 more 'F's to the mask to make it a 52-bit hash on the JS backend, but I think everyone agrees that, long-term, Nim 64-bit integers should be backed by JS BigInt not JS Number. At that point it can be the full 64-bits. } |
no need mention PR nim-lang#15915, fixed in nim-lang#15937
|
As a minor follow up to my {} paragraph above, if anyone does read that and expand the hash range, the |
|
@c-blake maybe instead add a comment in the code, otherwise this will be forgotten |
|
Well, I personally hope that |
* Alternate PR to nim-lang#15915 to resolve the problem mentioned there (`hash() == 0`) as well as to close nim-lang#15624 * Address nim-lang#15937 (comment) { though this was only a move from 2 copies to 3 copies. ;-) }
* Alternate PR to nim-lang#15915 to resolve the problem mentioned there (`hash() == 0`) as well as to close nim-lang#15624 * Address nim-lang#15937 (comment) { though this was only a move from 2 copies to 3 copies. ;-) }
* test for issue nim-lang#15624 and PR nim-lang#15915 for patch nim-lang#13823 * Update thashes.nim no need mention PR nim-lang#15915, fixed in nim-lang#15937 * rebase to devel(issue maybe fixed), ignore ouputs * Apply suggestions from code review Co-authored-by: flywind <43030857+xflywind@users.noreply.github.com>
* Alternate PR to nim-lang#15915 to resolve the problem mentioned there (`hash() == 0`) as well as to close nim-lang#15624 * Address nim-lang#15937 (comment) { though this was only a move from 2 copies to 3 copies. ;-) }
* Alternate PR to nim-lang#15915 to resolve the problem mentioned there (`hash() == 0`) as well as to close nim-lang#15624 * Address nim-lang#15937 (comment) { though this was only a move from 2 copies to 3 copies. ;-) }
* test for issue nim-lang#15624 and PR nim-lang#15915 for patch nim-lang#13823 * Update thashes.nim no need mention PR nim-lang#15915, fixed in nim-lang#15937 * rebase to devel(issue maybe fixed), ignore ouputs * Apply suggestions from code review Co-authored-by: flywind <43030857+xflywind@users.noreply.github.com>
JS VM and JS runtime hashes now match (and result is used) fixing the issue and the problem mentioned in the referred to PR.