This repository was archived by the owner on Nov 1, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 503
Enhance virtual function resolution logic to be roughly correct. #27
Merged
davidwrighton
merged 3 commits into
dotnet:master
from
davidwrighton:find_virtual_function
Oct 6, 2015
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,15 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <configuration> | ||
| <!-- NOTE: Leave this file here and keep it in sync with list in dir.props. --> | ||
| <!-- The command-line doesn't need it, but the IDE does. --> | ||
| <config> | ||
| <add key="repositoryPath" value="..\packages" /> | ||
| </config> | ||
| <packageSources> | ||
| <clear/> | ||
| <add key="myget.org dotnet-core" value="https://www.myget.org/F/dotnet-core/" /> | ||
| <add key="myget.org dotnet-coreclr" value="https://www.myget.org/F/dotnet-coreclr/" /> | ||
| <add key="myget.org dotnet-corefxtestdata" value="https://www.myget.org/F/dotnet-corefxtestdata/" /> | ||
| <add key="myget.org dotnet-buildtools" value="https://www.myget.org/F/dotnet-buildtools/" /> | ||
| <add key="nuget.org" value="https://www.nuget.org/api/v2/" /> | ||
| </packageSources> | ||
| <config> | ||
| <add key="repositoryPath" value="..\packages" /> | ||
| </config> | ||
| </configuration> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System; | ||
|
|
||
| namespace Internal.TypeSystem | ||
| { | ||
| public struct MethodImplRecord | ||
| { | ||
| public MethodDesc Decl; | ||
| public MethodDesc Body; | ||
| } | ||
|
|
||
| // MethodImpl api surface for types. | ||
| public partial class MetadataType | ||
| { | ||
| /// <summary> | ||
| /// Compute an array of all MethodImpls that pertain to overriding virtual (non-interface methods) on this type. | ||
| /// May be expensive. | ||
| /// </summary> | ||
| protected abstract MethodImplRecord[] ComputeVirtualMethodImplsForType(); | ||
|
|
||
| private MethodImplRecord[] _allVirtualMethodImplsForType; | ||
| /// <summary> | ||
| /// Get an array of all MethodImpls that pertain to overriding virtual (non-interface methods) on this type. | ||
| /// Expected to cache results so this api can be used repeatedly. | ||
| /// </summary> | ||
| public MethodImplRecord[] VirtualMethodImplsForType | ||
| { | ||
| get | ||
| { | ||
| if (_allVirtualMethodImplsForType == null) | ||
| { | ||
| _allVirtualMethodImplsForType = ComputeVirtualMethodImplsForType(); | ||
| } | ||
|
|
||
| return _allVirtualMethodImplsForType; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Get an array of MethodImpls where the Decl method matches by name with the specified name. | ||
| /// </summary> | ||
| /// <param name="name"></param> | ||
| /// <returns></returns> | ||
| public abstract MethodImplRecord[] FindMethodsImplWithMatchingDeclName(string name); | ||
| } | ||
|
|
||
| // Implementation of MethodImpl api surface implemented without metadata access. | ||
| public partial class InstantiatedType | ||
| { | ||
| /// <summary> | ||
| /// Instantiate a MethodImplRecord from uninstantiated form to instantiated form | ||
| /// </summary> | ||
| /// <param name="uninstMethodImpls"></param> | ||
| /// <returns></returns> | ||
| private MethodImplRecord[] InstantiateMethodImpls(MethodImplRecord[] uninstMethodImpls) | ||
| { | ||
| if (uninstMethodImpls.Length == 0) | ||
| return uninstMethodImpls; | ||
|
|
||
| MethodImplRecord[] instMethodImpls = new MethodImplRecord[uninstMethodImpls.Length]; | ||
|
|
||
| for (int i = 0; i < uninstMethodImpls.Length; i++) | ||
| { | ||
| instMethodImpls[i].Decl = _typeDef.Context.GetMethodForInstantiatedType(uninstMethodImpls[i].Decl, this); | ||
| instMethodImpls[i].Body = _typeDef.Context.GetMethodForInstantiatedType(uninstMethodImpls[i].Body, this); | ||
| } | ||
|
|
||
| return instMethodImpls; | ||
| } | ||
|
|
||
| protected override MethodImplRecord[] ComputeVirtualMethodImplsForType() | ||
| { | ||
| MethodImplRecord[] uninstMethodImpls = _typeDef.VirtualMethodImplsForType; | ||
| return InstantiateMethodImpls(uninstMethodImpls); | ||
| } | ||
|
|
||
| public override MethodImplRecord[] FindMethodsImplWithMatchingDeclName(string name) | ||
| { | ||
| MethodImplRecord[] uninstMethodImpls = _typeDef.FindMethodsImplWithMatchingDeclName(name); | ||
| return InstantiateMethodImpls(uninstMethodImpls); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We should decide on a convention here. FieldLayout related extensions to types all live in FieldLayout.cs. Can we put this in VirtualFunctionResolution.cs? Or should we align FieldLayout with this convention and put those things in TypeDesc.FieldLayout.cs?
My vote is to do nothing for this review, and fix up FieldLayout to align with the new convention introduced here.
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.
Yeah, I was having that thought too, and concluded that what I wanted in this case was to separate the Type system core functionality from the algorithm, as we may wish to implement load based virtual resolution someday, and it would be better to have the concepts separated to begin with. We can put adjusting the FieldLayout handling into future work.