-
-
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
std/hashes: hash(ref|ptr|pointer) + other improvements #17731
Conversation
e1f99c7
to
9a715dc
Compare
9a715dc
to
54b343c
Compare
how does this not break all code that defines a |
important_packages was green, and you can overload Note that any change (including pure bugfixes) is a potential breaking change for someone under some circumstance, but this shouldn't be a reason to freeze compiler/stdlib improvements. In future work (pending #12076 + another PR), you'll be able to selectively disable an overload |
this is hardly a signal, ie the nim language has many nooks and crannies in which bugs hide and symbol resolution is an unusually common one because the lookup rules - we've been here before, all too often - in the best case, this breaks something at compile time, in the worst case, it will break access to hash tables depending on which imports have been made and the mutual compatibility of the std lib declared hash function and the user-declared one.
this is why instead of taking a flippant approach to breaking user code, it's worth asking oneself about the mechanics of how a code change in such a central part as hash tables might break due to a change or why it will not break, and if it potentially will break things, notify users how they should work around the issue - it's also fine if you don't know or understand the language well enough to even determine these mechanics, but in this case it's even more important to ask the question before pulling the trigger. We have a large codebase that depends on these features and finding out about breaking changes after the fact has been a real problem for numerous releases and bugfix releases. If this were a fringe library, it wouldn't matter, but it isn't, really.
this does not help previously working code - it also doesn't help footguns such as slightly incompatible definitions of |
Yeah, it's the worst part of Nim IMHO. Time to bring up an RFC and attack the problem. We need concepts and we need to attach procs to types. |
@timotheecour since the RFC that would sort out these problems has not even been written yet (sorry), this extension must be opt-in first. |
hash(ref)
now supportedrefs https://forum.nim-lang.org/t/7765#49360
and doesn't make sense to me (1 of the tests in this PR would fail with this rule)
hash(pointer)
use the same scrambling as forhash(int)
(ie, honorsnimIntHash1
) so that code expecting scrambled hashes works with pointer (and ref|ptr) too, without performance drop, ie treats all those types that can be cast to int in an identical way; note that I still consider hashing collision: improve performance using linear/pseudorandom probing mix #13440 the superior, more performant alternative (see benchmarks) but that's a separate issue and can be addressed in future work when i revisit it)hash(closure)
; it was usinghash(rawProc(x)) !& hash(rawEnv(x))
which violates hash API's by not finalizing it with!$
(ie, can give bad distributions); instead I'm now usinghash((rawProc(x), rawEnv(x)))
which "does the right thing"asm """
p=
Data;"""
disableVm
future work
hash(ref|ptr|pointer)