Skip to content

Conversation

@JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Feb 8, 2024

Fixes: #9519, #7131

Context

Contains changes from:

Usage

<!-- Works unchanged. Identical to [MSBuild]::StableStringHash($x, 'Legacy') -->
[MSBuild]::StableStringHash($x)

<!-- 
  $hashAlgo will currently allow:
    'Legacy' - the legacy behavior (mimicking string.GetHashCode)
    'Fnv1a32bit' - Fawler-Noll-Vo 1a 32bit
    'Fnv1a32bitFast' - Custom, faster, Fawler-Noll-Vo 1a 32bit
    'Fnv1a64bit' - Fawler-Noll-Vo 1a 64bit
    'Fnv1a64bitFast' -  Custom, faster, Fawler-Noll-Vo 1a 64bit
    'Sha256' - hex string of the Sha256 hash of the given string
-->
[MSBuild]::StableStringHash($x, $hashAlgo)

Testing

  • Existing test on colissions extended for all overloads
  • Added test on expected output types

Documentation

Once PR content is agreed I'll create Doc bug + PR to update https://learn.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2022#msbuild-stablestringhash
We might not possibly document all the supported algos as of now.

@JanKrivanek JanKrivanek marked this pull request as ready for review February 8, 2024 13:43
@JanKrivanek
Copy link
Member Author

FYI @KirillOsenkov - this is using the faster versions of FNV from MSBuildStructuredLog (the source and your meassurements are linked in the code)

Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

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

LGTM.
Should we document this new feature somewhere?

Copy link
Contributor

@f-alizada f-alizada left a comment

Choose a reason for hiding this comment

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

LGTM!
Asking out of curiocity: Is there anything additionally that needs to be done in binarylog viewer after this is merged?

@JanKrivanek
Copy link
Member Author

LGTM! Asking out of curiocity: Is there anything additionally that needs to be done in binarylog viewer after this is merged?

Good question.
As a rule of thumb - Viewer needs changes when there is an addition of new derivative of BuildEventArgs or a change in definition of any existing derivatives of BuildEventArgs.
In this case we are adding property function overloads - this will not have any impact on BuildEventArgs

@JanKrivanek JanKrivanek merged commit 23f7752 into main Feb 16, 2024
@JanKrivanek JanKrivanek deleted the revert-9520-revert-stablestringhash branch February 16, 2024 11:27
JanKrivanek added a commit that referenced this pull request Feb 21, 2024
JanKrivanek added a commit that referenced this pull request Feb 22, 2024
@JanKrivanek JanKrivanek restored the revert-9520-revert-stablestringhash branch March 13, 2024 12:23
@JanKrivanek JanKrivanek deleted the revert-9520-revert-stablestringhash branch March 13, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce [MSBuild]::StableStringHash overload(s) with alternative hashing

4 participants