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

Fix hashing in Logging source gen #111073

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jan 3, 2025

Fixes #110698

This change to avoid throwing from the hashing method which can break logging source generation.

I'll submit another PR to fix https://github.com/dotnet/extensions/blob/20c12ef61fc33865f36c1f4f6e8e2240e8c25f32/src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs#L149 after I merge this one.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 3, 2025

@stephentoub could you please help review this change. I recall you helped review the original code.

CC @geeknoid

@tarekgh tarekgh added this to the 10.0.0 milestone Jan 3, 2025
@geeknoid
Copy link
Member

geeknoid commented Jan 3, 2025

Is this code supposed to actually be compiled with overflow checking on?

@tarekgh
Copy link
Member Author

tarekgh commented Jan 3, 2025

Is this code supposed to actually be compiled with overflow checking on?

My understanding is that overflowing check is turned on by default and we don't turn it off anywhere.

I misspoke here. Actually, the default behavior is unchecked according to the docs https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/checked-and-unchecked#default-overflow-checking-context. I'll remove the use of unchecked in the change then. We still need the remaining changes

@geeknoid
Copy link
Member

geeknoid commented Jan 4, 2025

Ah, it's Math.Abs that's throwing the exception. Two's complement strikes again!

@stephentoub stephentoub merged commit c242383 into dotnet:main Jan 6, 2025
81 of 83 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.

OverflowException in LoggerMessageGenerator.GetNonRandomizedHashCode
3 participants