Resolve ILLink warnings in System.Runtime.Serialization.Formatters #46964
Conversation
eerhardt
added
area-System.Runtime
linkable-framework
eerhardt
requested review from
tannergooding,
vitek-karas,
MichalStrehovsky and
joperezr
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding Issue DetailsContributes to #45623 cc @jeffhandley @bartonjs as listed area owners
|
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
| private List<MethodInfo>? GetMethodsWithAttribute(Type attribute, Type? t) | ||
| private List<MethodInfo>? GetMethodsWithAttribute( | ||
| Type attribute, | ||
| // currently the only way to preserve base, non-public methods is to use All |
There was a problem hiding this comment.
Is that something the linker plans to fix - if so should this be tracked somehow to fix later?
There was a problem hiding this comment.
I'm starting to wonder if PrivateMethods should be changed to mean private methods across the entire hierarchy - and not the current reflection semantics which is pretty confusing.
There was a problem hiding this comment.
I'm starting to wonder if PrivateMethods should be changed to mean private methods across the entire hierarchy
It's going to keep more stuff so the question is whether it's worth it. Matching reflection semantics currently means we keep exactly what "regular" reflection needs.
Looking at API usage statistics of Type.BaseType could be used as a first approximation - apps that don't use it would definitely not benefit from extending the semantics. Apps that use it might. Seems like Apisof.net no longer has usage statistics which is where I used to go looking for that stuff.
...ystem.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/FormatterServices.cs
Show resolved
Hide resolved
...es/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/ObjectManager.cs
Show resolved
Hide resolved
...aries/System.Runtime.Serialization.Formatters/ref/System.Runtime.Serialization.Formatters.cs
Outdated
Show resolved
Hide resolved
...aries/System.Runtime.Serialization.Formatters/ref/System.Runtime.Serialization.Formatters.cs
Outdated
Show resolved
Hide resolved
|
I believe all feedback has been addressed, if anyone wants to take another look. I'll merge this later today if I don't hear anything. |
|
Android failure is #47185 |
These were resolved with dotnet/runtime#46964
ghost
locked as resolved and limited conversation to collaborators
Contributes to #45623
cc @jeffhandley @bartonjs as listed area owners