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

List excluded reflection emit APIs #996

Merged
merged 2 commits into from Dec 6, 2018

Conversation

Projects
None yet
7 participants
@terrajobst
Member

terrajobst commented Dec 5, 2018

I did a diff between System.Reflection.Emit in .NET Framework and .NET Standard to make sure the APIs we explicitly didn't bring to .NET Standard are listed here. This adds no APIs to .NET Standard, just comments to the source code of the reference assembly.

I assume the @dotnet/nsboard has no problems with that as it's just book keeping for folks like me. Unless there is push back, I'm going to merge this tomorrow.

Update: I forgot to mention that I plan to add another PR with the APIs we plan to add .NET Standard.

terrajobst added some commits Dec 4, 2018

List excluded .NET Framework RefEmit APIs
The sole reason for this change is bookeeping so that we can record why
certain .NET Framework APIs weren't included.

@terrajobst terrajobst requested review from dotnet/nsboard-foundation as code owners Dec 5, 2018

@jkotas

This comment has been minimized.

Member

jkotas commented Dec 5, 2018

You may be missing DynamicILInfo that we plan to add back (dotnet/corefx#12055).

@terrajobst

This comment has been minimized.

Member

terrajobst commented Dec 5, 2018

You may be missing DynamicILInfo that we plan to add back (dotnet/corefx#12055).

Sorry for the confusion. There is a separate PR coming with the actual additions.

@jskeet

jskeet approved these changes Dec 5, 2018

Generally fine, just a couple of nits/suggestions.

@@ -39,6 +49,9 @@ public sealed partial class ConstructorBuilder : System.Reflection.ConstructorIn
public override System.Reflection.Module Module { get { throw null; } }
public override string Name { get { throw null; } }
public override System.Type ReflectedType { get { throw null; } }
// [System.ObsoleteAttribute("This property has been deprecated. http://go.microsoft.com/fwlink/?linkid=14202")]

This comment has been minimized.

@jskeet

jskeet Dec 5, 2018

Is it useful to maintain the attribute in commented form? If it makes diffs simpler, that's fine - I'm not sure how useful it is otherwise. (Unless it also serves as the reason for it to not be included? That would make sense.)

This comment has been minimized.

@terrajobst

terrajobst Dec 6, 2018

Member

Commented APIs should have a justification. It seemed to me the obsolete attribute was as good as any other justification but with the added benefit that it was already available in Beyond Compare :-)

@@ -39,6 +49,9 @@ public sealed partial class ConstructorBuilder : System.Reflection.ConstructorIn
public override System.Reflection.Module Module { get { throw null; } }
public override string Name { get { throw null; } }
public override System.Type ReflectedType { get { throw null; } }
// [System.ObsoleteAttribute("This property has been deprecated. http://go.microsoft.com/fwlink/?linkid=14202")]
// public System.Type ReturnType { get { throw null; } }
//CAS public void AddDeclarativeSecurity(System.Security.Permissions.SecurityAction action, System.Security.PermissionSet pset) { }

This comment has been minimized.

@jskeet

jskeet Dec 5, 2018

I assume "//CAS" is shorthand for "// Excluded because we don't support Code Access Security"? It might be simpler to keep the consistent format across reasons.

This comment has been minimized.

@terrajobst

terrajobst Dec 6, 2018

Member

Yes, we're using CAS as the exclusion reason all over the place already. At least on our end that term is well understood.

@terrajobst

This comment has been minimized.

Member

terrajobst commented Dec 6, 2018

Since it seems everyone is fine with these, I'm going to merge this.

@terrajobst terrajobst merged commit 689aaa9 into dotnet:master Dec 6, 2018

5 checks passed

OSX10.12 Build finished.
Details
Ubuntu16.04 Build finished.
Details
Windows_NT Build finished.
Details
license/cla All CLA requirements met.
Details
standard-ci #20181204.6 succeeded
Details

@terrajobst terrajobst deleted the terrajobst:reflection-sync branch Dec 6, 2018

@terrajobst

This comment has been minimized.

Member

terrajobst commented Dec 6, 2018

Sigh. Didn't push the typo fixes to the PR before merging. Instead I pushed directly to master as part of 4b8e411.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment