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

Options Source Gen Fixes #91363

Merged
merged 4 commits into from
Aug 31, 2023
Merged

Options Source Gen Fixes #91363

merged 4 commits into from
Aug 31, 2023

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Aug 30, 2023

Planning to port these fixes to rc2

The changes in this PR fix the following issues:

@ghost
Copy link

ghost commented Aug 30, 2023

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

Planning to port these fixes to rc2

The changes in this PR fix the following issues:

Author: tarekgh
Assignees: tarekgh
Labels:

area-Extensions-Options

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone Aug 30, 2023
@tarekgh tarekgh added the source-generator Indicates an issue with a source generator feature label Aug 30, 2023
@tarekgh
Copy link
Member Author

tarekgh commented Aug 30, 2023

CC @jeffhandley @geeknoid

Comment on lines +449 to +461
/// <summary>
/// Returns a non-randomized hash code for the given string.
/// We always return a positive value.
/// </summary>
internal static int GetNonRandomizedHashCode(string s)
{
uint result = 2166136261u;
foreach (char c in s)
{
result = (c ^ result) * 16777619;
}
return Math.Abs((int)result);
}
Copy link
Member

Choose a reason for hiding this comment

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

We might want to consider using SHA256 here instead of FNV-1a. I think that's what the compiler itself uses when it needs uniqueness.

Copy link
Member Author

Choose a reason for hiding this comment

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

After offline discussion, we decided to stick with FNV-1a.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice. Thanks.

@tarekgh
Copy link
Member Author

tarekgh commented Aug 31, 2023

Failing tests tracked by #91410

@tarekgh
Copy link
Member Author

tarekgh commented Aug 31, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6043270790

@tarekgh tarekgh merged commit a8d5e7d into dotnet:main Aug 31, 2023
105 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants