SWEEP: Add unchecked to GetHashCode, #1065

[paulirwin]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Adds unchecked to all GetHashCode implementations where it is applicable

Fixes #1065

Description

GetHashCode should not throw an exception on overflow. This PR adds unchecked blocks to all GetHashCode implementations that do addition or multiplication that could overflow.

During review, it was determined that GetHashCode methods that return constants, simply delegate GetHashCode to another object, or solely do bitwise xor operations do not need unchecked added.

[NightOwl888]

Thanks for the PR. It looks good.

There are 3 places where we are now doubling up on the unchecked block with an unchecked cast in the middle. We can now remove the cast. See the inline comments.

[NightOwl888]

Wouldn't this be categorized as a performance improvement rather than a bugfix?

[paulirwin]

Wouldn't this be categorized as a performance improvement rather than a bugfix?

I'm not sure there would be any performance impact. Originally I had assumed that we needed to do this to avoid overflow exceptions, but I realized just now that our projects do not have the "Check for arithmetic overflow" compiler option checked, so that reasoning is not valid alone. Perhaps an argument could be made that it helps ensure best practices and might help users if they want to recompile the library with that option for some reason, as well as to be consistent with other places in the codebase where we were already using unchecked blocks in GetHashCode... but in terms of performance, it looks like it generates the same opcodes without these blocks (i.e. add vs add.ovf), so there's no perf benefit here either. I'm not opposed to still moving forward with this PR for consistency's sake, but it's not the bugfix I thought it was. Perhaps "notes:ignore" would be a better label then.

[paulirwin]

Aside: we seem to be more regularly be getting transient build failures downloading from nuget.org and MyGet again. This possibly is a result of adding the .NET 9 tests with an extra ~70 actions running in parallel. Fortunately the workaround is simple, to just choose the option to re-run failed jobs.

[NightOwl888]

Aside: we seem to be more regularly be getting transient build failures downloading from nuget.org and MyGet again. This possibly is a result of adding the .NET 9 tests with an extra ~70 actions running in parallel. Fortunately the workaround is simple, to just choose the option to re-run failed jobs.

I suspect this is due to extra holiday traffic, however, it would probably help if we enabled NuGet caching (#856 and #857). I am not convinced we need to generate an extra file and figure out how to maintain it just so we can update packages, though. Our first attempt at doing so (on Azure DevOps) didn't need to, but the key didn't include the dependencies.props file (which missed the entire point) and apparently the only way to fix that is to completely rename the cache key so it can include that file (it should include all .props, .targets, and '.*proj` files just to be on the safe side).