Skip to content
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

Hash compare by identity #8451

Merged
merged 5 commits into from Nov 8, 2019
Merged

Conversation

@asterite
Copy link
Member

asterite commented Nov 7, 2019

Implements #8329

Has a commit from #8450 but I can later rebase on top of master, or just rewrite git history.

src/hash.cr Outdated

private def entry_matches?(entry, key)
entry_key = entry.key
if @compare_by_identity && entry_key.responds_to?(:object_id)

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Nov 7, 2019

Member

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?

This comment has been minimized.

Copy link
@asterite

asterite Nov 7, 2019

Author Member

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.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Nov 7, 2019

Member

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 is false
  • when !entry_key.responds_to?(:object_id) && key.responds_to?(:object_id) the result is entry_key == key

This comment has been minimized.

Copy link
@asterite

asterite Nov 7, 2019

Author Member

Sounds good, I'll at least make it consistent

This comment has been minimized.

Copy link
@asterite

asterite Nov 7, 2019

Author Member

Done! Let me know what you think.

@asterite asterite force-pushed the asterite:hash-compare-by-identity branch from 1dc88ec to 860067c Nov 7, 2019
Copy link
Member

bcardiff left a comment

Shouldn't dup and clone preserve the compare_by_identity?

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 7, 2019

Shouldn't dup and clone preserve the compare_by_identity?

Great catch! Done

@asterite asterite added this to the 0.32.0 milestone Nov 8, 2019
@asterite asterite merged commit 8459700 into crystal-lang:master Nov 8, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite asterite deleted the asterite:hash-compare-by-identity branch Nov 8, 2019
@bew

This comment has been minimized.

Copy link
Contributor

bew commented Dec 11, 2019

Is it intentional to not have a way to switch back to "compare by hash" ?

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Dec 11, 2019

Switching back an forth on the same instance would be confusing. compare_by_identity should be configured only once and prior to any usage of the hash. Ideally, that's in the constructor and I think we should actually make it a constructor argument (see discussion in #8329).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.