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

Try fix GetHashCode implementation of the Thickness struct is broken for uniform length #8822

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lindexi
Copy link
Contributor

@lindexi lindexi commented Feb 18, 2024

Fixes #8789

Description

I change the GetHashCode implementation of the Thickness struct. I use the HashCode to calculate the hash code of Thickness.

I'm not sure I'm doing the right thing.

Customer Impact

The GetHashCode of Thickness will be change

Regression

Testing

Just CI.

Risk

Normal. It's hard to evaluate. Because some logic that depends on the original behavior can break down.

Microsoft Reviewers: Open in CodeFlow

@lindexi lindexi requested a review from a team as a code owner February 18, 2024 07:22
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Feb 18, 2024
@miloush
Copy link
Contributor

miloush commented Feb 18, 2024

Hash codes are explicitly not guaranteed to remain the same not only across framework versions but even across runs of the same application.

This might be a better way to do this:
https://github.com/dotnet/winforms/blob/2441a36314a2312e219278e0459ba0aef8a2b7f6/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Padding.cs#L167

@lindexi
Copy link
Contributor Author

lindexi commented Feb 18, 2024

Thank you @miloush and I will use your code.

@halgab
Copy link
Contributor

halgab commented Feb 21, 2024

Apparently there are many other places that could switch to such a pattern. Should the scope of this PR be broadened? Or perhaps we could open a follow-up issue to review the GetHashCode implementations across the repo?

@lindexi
Copy link
Contributor Author

lindexi commented Feb 22, 2024

@halgab At first, I must sure I do the right thing. I do not know change the GetHashCode behavior is the right thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The GetHashCode implementation of the Thickness struct is broken for uniform length.
3 participants