Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Remove usage of EqualityComparer from ValueTuple's GetHashCode #14187

Closed
wants to merge 1 commit into from

Conversation

jamesqo
Copy link
Contributor

@jamesqo jamesqo commented Dec 4, 2016

EqualityComparer<T>.Default.GetHashCode(item) has exactly the same behavior as item?.GetHashCode ?? 0. The only difference is that the latter version is faster because it is not an extra call.

Previously, the only compelling reason to use the former was that if item was an enum, then calling GetHashCode directly would box while EqualityComparer wouldn't (see here).

However, this change in coreclr and this one in corert eliminate boxing for enums, so there is no longer a reason to use EqualityComparer in ValueTuple. This PR replaces all usage of it with item?.GetHashCode() ?? 0. (I used a regex find-and-replace.)

cc @VSadov @jcouv @stephentoub @omariom

Fixes https://github.com/dotnet/corefx/issues/13774

@jkotas
Copy link
Member

jkotas commented Dec 4, 2016

The corefx version of ValueTuple is really only going be just for existing runtimes (.NET Framework, .NET Core 1.0). The existing runtimes do not have the JIT optimization that this depends on.

@jcouv has a workitem to add the ValueTuple to CoreLib for CoreCLR. This change should wait until then, and it should be done only on the CoreLib copy.

@stephentoub
Copy link
Member

Closing per @jkotas' comments. Thanks.

@stephentoub stephentoub closed this Dec 4, 2016
@jamesqo jamesqo deleted the gethashcode branch December 4, 2016 19:00
@karelz karelz added this to the 1.2.0 milestone Dec 6, 2016
@jcouv
Copy link
Member

jcouv commented Dec 6, 2016

FYI Adding ValueTuple to CoreLib for CoreCLR is tracked by umbrella issue dotnet/roslyn#13177

@jamesqo
Copy link
Contributor Author

jamesqo commented Dec 7, 2016

@jcouv Ok, thanks for telling me. I have posted a comment in that thread so everything is in one place.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants