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

Implement IEquatable on structs. #17088

Merged
merged 1 commit into from Sep 15, 2019
Merged

Implement IEquatable on structs. #17088

merged 1 commit into from Sep 15, 2019

Conversation

@RoosterDragon
Copy link
Member

RoosterDragon commented Sep 13, 2019

Any struct which overrides object.Equals(object obj) should implement IEquatable to gain a more efficient Equals(T other) overload. This overload will be used by hashing collections like Dictionary which enables them to check equality without boxing the struct.

(As a bonus, since we now own Color, we can remove the hack in Pair for a better comparer, since we already have one!)

Any struct which overrides object.Equals(object obj) should implement IEquatable<T> to gain a more efficient Equals(T other) overload. This overload will be used by hashing collections like Dictionary which enables them to check equality without boxing the struct.
Copy link
Member

abcdefg30 left a comment

Code changes make sense.

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Sep 15, 2019

Just a question regarding the GetHashCode() implementation on these structs. I often see GetHashCode() implemented using prime numbers to reduce the chance of collisions, is this something we should do too?

@RoosterDragon

This comment has been minimized.

Copy link
Member Author

RoosterDragon commented Sep 15, 2019

Yeah the GetHashCode() methods that just xor everything aren't great.

We'd want to analyse the sorts of values that tend to actually end up in HashSet or Dictionaries, and then try and come up with a hash function that reduces collisions for those common values. Throwing in some primes or a few bitshifts would likely help.

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Sep 15, 2019

Lets leave that for another PR.

@teinarss teinarss merged commit 6c9fbd4 into OpenRA:bleed Sep 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RoosterDragon RoosterDragon deleted the RoosterDragon:equals branch Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.