Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Step 1 of fix for disabled System.Dynamic.Runtime test on ILC #20991

Merged
merged 2 commits into from Jun 14, 2017
Merged

Step 1 of fix for disabled System.Dynamic.Runtime test on ILC #20991

merged 2 commits into from Jun 14, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jun 13, 2017

(https://github.com/dotnet/corefx/issues/19895)

Make Microsoft.CSharp use the the real HasMetadataDefinitionAs()
api if it exists on the running framework. Since this library
is built on netstandard, it still needs to fall back on its
emulation on older frameworks.

Actual removal of the ActiveIssue will have to wait until
the new System.Runtime reference assembly propagates to
the Project N tree and the new api becomes reflectable.

(https://github.com/dotnet/corefx/issues/19895)

Make Microsoft.CSharp use the the real HasMetadataDefinitionAs()
api if it exists on the running framework. Since this library
is built on netstandard, it still needs to fall back on its
emulation on older frameworks.

Actual removal of the ActiveIssue will have to wait until
the new System.Runtime reference assembly propagates to
the Project N tree and the new api becomes reflectable.
@ghost ghost assigned VSadov Jun 13, 2017
@ghost ghost requested a review from VSadov June 13, 2017 14:54
@@ -278,11 +278,36 @@ private static bool IsTypeParameterEquivalentToTypeInst(this Type typeParam, Typ

public static bool HasSameMetadataDefinitionAs(this MemberInfo mi1, MemberInfo mi2)
{
if (s_lazyHasSameMetadataDefinitionAsApi == null)
{
MethodInfo apiMethod = typeof(MemberInfo).GetMethod(
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be added to the self-modifying s_MemberEquivalence above. It woudl be just another try/catch before we check for the MetadataToken.

Note that the pattern not only checks for the presence of the API, but also that it does not throw right away. We had major issues with API being discovered and then throwing something like "Not supported" or "Method not avaialble" - I do not remember.
The pattern is to
1 - probe for the API
2 - try using the API for the given args
3 - if API worked without exceptions, self-modify the func to the new implementation so we are no longer wasting time on various conditional checks.
4 - return the result.

Copy link
Member

Choose a reason for hiding this comment

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

"Before the probing for MetadataToken" is becasue I think this would be more efficient if both are present.


In reply to: 121757009 [](ancestors = 121757009)

Copy link
Member

@VSadov VSadov Jun 13, 2017

Choose a reason for hiding this comment

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

I'd expect it to be something like:

try
{
         MethodInfo apiMethod = typeof(MemberInfo).GetMethod(
                    "HasSameMetadataDefinitionAs",
                    BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly | BindingFlags.ExactBinding,
                    binder: null,
                    types: new Type[] { typeof(MemberInfo) },
                    modifiers: null);

        if (apiMethod != null)
        {
               var sameDefAs  = (Func<MemberInfo, MemberInfo, bool>)Delegate.CreateDelegate(typeof(Func<MemberInfo, MemberInfo, bool>), apiMethod);

               var result = sameDefAs  (m1, m2);

               // it worked, so publish it
               s_MemberEquivalence = sameDefAs;
               return result;
        }
}
catch
{
          // Platform might not allow access to the API or it is not implemented.
}

In reply to: 121757257 [](ancestors = 121757257,121757009)

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

The new API should just be probed for in the same place where we probe/bind other possible implementations of s_MemberEquivalence.

And we should probe for this one first, since it seems to be preferable when present.

@ghost
Copy link
Author

ghost commented Jun 13, 2017

Merge probe with the existing probes.

@ghost
Copy link
Author

ghost commented Jun 13, 2017

@dotnet-bot test Portable Linux x64 Debug Build

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks!!

@VSadov VSadov merged commit 4b830e6 into dotnet:master Jun 14, 2017
@ghost ghost deleted the dr branch June 14, 2017 13:46
@karelz karelz modified the milestone: 2.1.0 Jun 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants