-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Remove unnecessary calls to GetTypeInfo #27974
Conversation
{ | ||
if (!constructor.IsStatic && constructor.IsPublic) | ||
if (!constructor.IsStatic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note these changes match what was done in dotnet/runtimes ActivatorUtilities. See https://github.com/dotnet/runtime/pull/40227/files#diff-d04fd33a5eba20dc5b742e17b751d3b9c0860d114b73c777a0e140c1ceaccf6fR57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm when?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is constructor.IsPublic
implied? Or does this change ActivatorUtilities.CreateInstance to use non-public ctors? If it's the latter, this seems like a fairly major breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous conversation about this: https://github.com/dotnet/runtime/pull/40227/files#r463897549
TLDR: GetConstructors
returns only public constructors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked again, it also only returns Instance
constructors, so we could remove the IsStatic
check from both copies (runtime and aspnetcore) of this code.
Throwing a bunch of areas on for people to take a look. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
GetTypeInfo() calls are no longer necessary just to use Reflection APIs. And typically they allocate a small
TypeDelegator
object when used, so they should be avoided when not necessary.I removed the obvious cases from the product, and a few occurrences in
test
code. However, I didn't tackle every placetest
code was calling it as there is a diminishing return.Similar work is done in dotnet/runtime: