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

Sgen: Update GetTempAssemblyName according to #46499 #57026

Merged
merged 5 commits into from
Aug 17, 2021
Merged

Sgen: Update GetTempAssemblyName according to #46499 #57026

merged 5 commits into from
Aug 17, 2021

Conversation

TalAloni
Copy link
Contributor

@TalAloni TalAloni commented Aug 7, 2021

In #46499 we corrected Compilation.GetTempAssemblyName in order for it to be not deterministic under .NET Core.
In this PR we update the generated filename to match the new logic.

In #46499 we corrected Compilation.GetTempAssemblyName in order for it to be not deterministic under .NET Core.
In this PR we update the generated filename to match the new logic.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 7, 2021
@TalAloni TalAloni changed the title Update GetTempAssemblyName according to #46499 SGen: Update GetTempAssemblyName according to #46499 Aug 7, 2021
@TalAloni TalAloni changed the title SGen: Update GetTempAssemblyName according to #46499 Sgen: Update GetTempAssemblyName according to #46499 Aug 7, 2021
Copy link
Member

@StephenMolloy StephenMolloy left a comment

Choose a reason for hiding this comment

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

Question for @HongGit about the target framework, but otherwise this looks good. I'll let Hong have a chance to review and merge.

return ReadUInt32BigEndian(hash);
}

private static uint ReadUInt32BigEndian(byte[] value)
Copy link
Member

Choose a reason for hiding this comment

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

I assume we include our own implementation of ReadUInt32BigEndian() here because the framework version is netstandard2.1, while this tool targets netstandard2.0? If that's the case, I think this is fine.

@HongGit, do we have any plans to update the targetframework for this tool, or are we sticking to netstandard2.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, we are still sticking to netstandard2.0. If there is a need to update, we could consider to.

@StephenMolloy StephenMolloy merged commit 4699992 into dotnet:main Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Serialization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants