Share portions of CoreCLR and Mono CoreLib's ILLinkTrim.xml file #37996
Conversation
eerhardt
requested review from
akoeplinger,
EgorBo,
steveisok and
vargaz
as code owners
|
Tagging subscribers to this area: @ViktorHofer |
|
Tagging subscribers to this area: @safern, @ViktorHofer |
| @@ -0,0 +1,63 @@ | |||
| <linker> | |||
There was a problem hiding this comment.
Is .Shared suffix necessary?
There was a problem hiding this comment.
It technically isn't necessary, but since ILLink.Descriptors.xml is the full name of the resource embedded in the assembly, I thought it would be confusing to someone who thought this was the full descriptor file. Putting Shared in the file name qualifies it being the "shared" pieces, and not the full file.
| @@ -753,71 +753,10 @@ | |||
| <method signature="System.Void .ctor()" /> | |||
There was a problem hiding this comment.
Could you also rename the file to ILLink.Descriptors.xml ?
There was a problem hiding this comment.
I was going to do that as part of #37651.
| <assembly fullname="System.Private.CoreLib"> | ||
| <type fullname="Interop/Globalization"> | ||
| <!-- Internal API used by tests only. --> | ||
| <method name="GetICUVersion" /> |
There was a problem hiding this comment.
Separate from this PR, if this is only used by tests, can it be part of _LibraryBuild file instead?
There was a problem hiding this comment.
I looked at this yesterday,
runtime/src/libraries/Common/src/Interop/Interop.ICU.cs
Lines 27 to 28 in a9626b9
I was thinking of just moving the P/Invoke to the tests.
But now that I look again, our Globalization.Native shim is replaced with QCall -- can you do that from an assembly outside of S.P.Corelib?
If not, then yes it could be moved to _LibraryBuild.
This reduces the duplication between these libraries, and allows for easier maintenance going forward. Fix dotnet#37255
eerhardt
force-pushed
the
CombineSPCILLinkTrim
branch
from
5c19ef7 to
f50bc09
Compare
ghost
locked as resolved and limited conversation to collaborators
This reduces the duplication between these libraries, and allows for easier maintenance going forward.
Fix #37255