Hash compare by identity #8451
Hash compare by identity #8451
Conversation
src/hash.cr
Outdated
|
|
||
| private def entry_matches?(entry, key) | ||
| entry_key = entry.key | ||
| if @compare_by_identity && entry_key.responds_to?(:object_id) |
There was a problem hiding this comment.
What if @compare_by_identity is true and entry responds to object_id but entry_key doesn't? Shouldn't that also return false even if entry_key == key?
There was a problem hiding this comment.
I don't know. If entry_key is an int you still want ints to compare the same way.
I also thought about raising a compile-time error when using compare_by_identity on a hash where keys don't respond to object_id, but it involved a chunk of macro code. Would something like that be better?
In general, I wouldn't worry too much about this. If you use compare_by_identity you generally know what you are doing and you'll be already working with reference types, or reference-like types. But we can try adding the macro check.
There was a problem hiding this comment.
I'm not sure what's the best semantics here. But I'd try to be consistent.
Currently, when comparing by identity the behaviour for differing answers to responds_to?(:object_id) is inconsistent:
- when
entry_key.responds_to?(:object_id) && !key.responds_to?(:object_id)the result isfalse - when
!entry_key.responds_to?(:object_id) && key.responds_to?(:object_id)the result isentry_key == key
There was a problem hiding this comment.
Sounds good, I'll at least make it consistent
There was a problem hiding this comment.
Done! Let me know what you think.
asterite
force-pushed
the
hash-compare-by-identity
branch
from
1dc88ec to
860067c
Compare
Great catch! Done |
|
Is it intentional to not have a way to switch back to "compare by hash" ? |
|
Switching back an forth on the same instance would be confusing. |
Implements #8329
Has a commit from #8450 but I can later rebase on top of
master, or just rewrite git history.