Skip to content

Conversation

kg
Copy link
Member

@kg kg commented Sep 16, 2025

Not sure why this unused parameter is here, though. Looks like a bug that it's not used?

@Copilot Copilot AI review requested due to automatic review settings September 16, 2025 16:27
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 16, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR suppresses an IDE0060 warning (unused parameter) for the seed parameter in the HashCodeBuilder constructor. The author notes this appears to be a bug since the parameter is not used in the implementation.

Key Changes

  • Adds pragma directives to suppress IDE0060 warning around the constructor
  • No functional code changes

@kg kg requested a review from MichalStrehovsky September 16, 2025 16:27
@xtqqczze
Copy link
Contributor

xtqqczze commented Sep 16, 2025

Not sure why this unused parameter is here

Introduced in #119385, replacing the previous implementation:

public HashCodeBuilder(string seed)
{
    _hash1 = 0x6DA3B944;
    _hash2 = 0;
    _numCharactersHashed = 0;

    Append(seed);
}

cc: @MichalStrehovsky

@tarekgh
Copy link
Member

tarekgh commented Sep 16, 2025

@jkotas can this get merged? It looks like it's causing a lot of failures in CI. @MichalStrehovsky looks offline.

@kg
Copy link
Member Author

kg commented Sep 16, 2025

I'm open to also adding the missing Append() call since it looks like that's the correct fix.

@xtqqczze
Copy link
Contributor

I'm open to also adding the missing Append() call since it looks like that's the correct fix.

It does look like the correct fix. As far as I can tell, it doesn’t currently impact behavior, since the seed span is empty at all of the call sites.

@tarekgh
Copy link
Member

tarekgh commented Sep 16, 2025

Maybe we can add it under a condition checking the seed is not empty? or if Append(seed); is not affected by empty span, then we can call it anyway in case in the future we can get non-empty span.

@xtqqczze
Copy link
Contributor

Maybe we can add it under a condition checking the seed is not empty? or if Append(seed); is not affected by empty span, then we can call it anyway in case in the future we can get non-empty span.

It makes no difference to behavior:

public void Append(ReadOnlySpan<byte> src)
{
if (src.Length == 0)
return;

@tarekgh
Copy link
Member

tarekgh commented Sep 16, 2025

Let's call Append anyway then.

@kg
Copy link
Member Author

kg commented Sep 16, 2025

Let's call Append anyway then.

Done

@kg
Copy link
Member Author

kg commented Sep 16, 2025

Should I ba-g to unblock people? I verified it builds locally but haven't run tests.

@tarekgh tarekgh added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 16, 2025
@tarekgh tarekgh added this to the 11.0.0 milestone Sep 16, 2025
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@xtqqczze
Copy link
Contributor

xtqqczze commented Sep 16, 2025

Should I ba-g to unblock people? I verified it builds locally but haven't run tests.

All the call sites are inside an internal class. I think there is fairly obviously no change in behavior, see grep output for HashCodeBuilder:

src/coreclr/tools/Common/Internal/Metadata/NativeFormat/MetadataTypeHashingAlgorithms.cs
6:using HashCodeBuilder = Internal.VersionResilientHashCode.HashCodeBuilder;
14:        private static void AppendNamespaceHashCode(ref HashCodeBuilder builder, NamespaceDefinitionHandle namespaceDefHandle, MetadataReader reader, bool appendDot)
35:        private static void AppendNamespaceHashCode(ref HashCodeBuilder builder, NamespaceReferenceHandle namespaceRefHandle, MetadataReader reader, bool appendDot)
60:            HashCodeBuilder builder = new HashCodeBuilder(""u8);
82:            HashCodeBuilder builder = new HashCodeBuilder(""u8);

src/libraries/Common/src/Internal/VersionResilientHashCode.cs
17:        public struct HashCodeBuilder
23:            public HashCodeBuilder(ReadOnlySpan<byte> seed)

@kg
Copy link
Member Author

kg commented Sep 16, 2025

/ba-g Unblocking CI, change is behavior-preserving

@kg kg merged commit 579bd79 into dotnet:main Sep 16, 2025
33 of 79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants