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.

Sign up for GitHub

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

Jump to bottom

Simplify AssemblyName marshalling between VM and CoreLib #68735

Merged
merged 1 commit into from
May 1, 2022
Merged

Simplify AssemblyName marshalling between VM and CoreLib #68735

merged 1 commit into from
May 1, 2022

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Apr 30, 2022

  • Introduce NativeAssemblyNameParts that is unmanaged view for the managed AssemblyNameParts
  • Use NativeAssemblyNameParts to convert manually managed code to C#

@ghost ghost assigned jkotas Apr 30, 2022
@ghost
Copy link

ghost commented Apr 30, 2022

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

Issue Details
  • Introduce NativeAssemblyNameParts that is unmanaged view for the managed AssemblyNameParts
  • Use NativeAssemblyNameParts to convert manually managed code to C#
Author: jkotas
Assignees: jkotas
Labels:

area-AssemblyLoader-coreclr
Milestone: -

@jkotas jkotas force-pushed the assembly-name-parts branch from 123df0c to 90c3209 Compare April 30, 2022 15:22
jkotas
jkotas commented Apr 30, 2022
View reviewed changes
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

AssemblyNameFlags flags = assemblyName.RawFlags;

// Note that we prefer to take a public key token if present,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is mirroring the logic that used to be in C++

@jkotas jkotas force-pushed the assembly-name-parts branch from b2e096c to 0793a7e Compare April 30, 2022 16:41
@jkotas
Copy link
Member Author

jkotas commented Apr 30, 2022

This is contributing towards moving more of assembly loader to C#. The next step after this PR is going to switch assembly name parsing to be C# only and delete the C++ parser.

@jkotas jkotas mentioned this pull request Apr 30, 2022
Closed
@jkotas jkotas force-pushed the assembly-name-parts branch from 9b1d81c to b8ccd39 Compare April 30, 2022 21:56
@jkotas
Simplify AssemblyName marshalling between VM and CoreLib
a18c80e 
- Introduce NativeAssemblyNameParts that is unmanaged view for the managed AssemblyNameParts
- Use NativeAssemblyNameParts to convert manually managed code to C#
- Delete unnecessary Version allocation
@jkotas jkotas force-pushed the assembly-name-parts branch from b8ccd39 to a18c80e Compare April 30, 2022 22:07
@jkotas jkotas mentioned this pull request Apr 30, 2022
Merged
@jkotas
Copy link
Member Author

jkotas commented May 1, 2022

This is ready for review.

@denisemel9
Copy link

denisemel9 commented May 1, 2022 via email

VSadov
VSadov approved these changes May 1, 2022
View reviewed changes
Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM!

@jkotas jkotas merged commit 3535e07 into dotnet:main May 1, 2022
@jkotas jkotas deleted the assembly-name-parts branch May 1, 2022 14:35
@DrewScoggins
Copy link
Member

Improvement: dotnet/perf-autofiling-issues#5076

@AndyAyersMS
Copy link
Member

AndyAyersMS commented May 12, 2022 •
edited
Loading

Improvement: dotnet/perf-autofiling-issues#5276, dotnet/perf-autofiling-issues#5275, dotnet/perf-autofiling-issues#5098

@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@VSadov VSadov VSadov approved these changes

Assignees

@jkotas jkotas

Labels
area-AssemblyLoader-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jkotas @denisemel9 @DrewScoggins @AndyAyersMS @stephentoub @VSadov