Skip to content

Always mark Delegate types as instantiated#454

Merged
marek-safar merged 1 commit into
dotnet:masterfrom
Unity-Technologies:master-always-mark-del-as-inst-possible
Feb 4, 2019
Merged

Always mark Delegate types as instantiated#454
marek-safar merged 1 commit into
dotnet:masterfrom
Unity-Technologies:master-always-mark-del-as-inst-possible

Conversation

@mrvoorhe
Copy link
Copy Markdown
Contributor

@mrvoorhe mrvoorhe commented Feb 1, 2019

Delegate and MulticastDelegate are instantiated from native code which causes us to not record that instances of these types could exist. As a result, today we remove the interfaces from Delegate. Removing the interfaces doesn't seem to cause any issues today, but it's probably not the greatest idea.

In the future with base sweeping or uninstantiated type method stubbing, this lack of recording these types as instance possible causes bigger problems.

@mrvoorhe mrvoorhe requested a review from marek-safar as a code owner February 1, 2019 16:35
Comment thread linker/Linker.Steps/MarkStep.cs Outdated

protected virtual bool AlwaysMarkTypeAsInstantiated (TypeDefinition td)
{
switch (td.FullName) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FullName is expensive property, use name + namespace instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mrvoorhe mrvoorhe force-pushed the master-always-mark-del-as-inst-possible branch from c9a295b to 7273418 Compare February 1, 2019 18:39
This was referenced Feb 1, 2019
@marek-safar
Copy link
Copy Markdown
Contributor

build

/// <summary>
/// Delegate and is created from
/// </summary>
[SetupLinkerCoreAction ("link")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will be problematic as the mscorlib will be used for execution.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don’t follow. Are you saying that it’s bad that this test is dependent on a particular mscorlib?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am saying it won't be easy to run the test

Delegate and MulticastDelegate are instantiated from native code which causes us to not record that instances of these types could exist.  As a result, today we remove the interfaces from Delegate.  This doesn't seem to cause any issues, but it's probably not the greatest idea.

In the future with base sweeping or uninstantiated type method stubbing, this lack of recording these types as instance possible causes bigger problems.
@mrvoorhe mrvoorhe force-pushed the master-always-mark-del-as-inst-possible branch from 7273418 to 9a543b9 Compare February 4, 2019 14:01
@marek-safar marek-safar merged commit a887149 into dotnet:master Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants