Skip to content
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

Add back EnumBuilder.CreateType() #46681

Closed
eerhardt opened this issue Jan 7, 2021 · 5 comments · Fixed by #49319
Closed

Add back EnumBuilder.CreateType() #46681

eerhardt opened this issue Jan 7, 2021 · 5 comments · Fixed by #49319
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Emit
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Jan 7, 2021

Background and Motivation

.NET Framework has the following APIs:

System.Type TypeBuilder.CreateType()
System.Reflection.TypeInfo TypeBuilder.CreateTypeInfo()

System.Type EnumBuilder.CreateType()
System.Reflection.TypeInfo EnumBuilder.CreateTypeInfo()

In .NET 5 we have:

System.Type? TypeBuilder.CreateType()
System.Reflection.TypeInfo? TypeBuilder.CreateTypeInfo()

System.Reflection.TypeInfo? EnumBuilder.CreateTypeInfo()

To be consistent with both .NET Framework, and between EnumBuilder and TypeBuilder, we should add back the CreateType() method on EnumBuilder.

Proposed API

namespace System.Reflection.Emit
{
    public sealed class EnumBuilder : System.Reflection.TypeInfo
    {
+       public System.Type? CreateType() { throw null; }
        public System.Reflection.TypeInfo? CreateTypeInfo() { throw null; }
     }

Usage Examples

        EnumBuilder eb = ...;

        // Define two members, "High" and "Low".
        eb.DefineLiteral("Low", 0);
        eb.DefineLiteral("High", 1);

        // Create the type and save the assembly.
        Type finished = eb.CreateType();

cc @terrajobst

@eerhardt eerhardt added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection.Emit labels Jan 7, 2021
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 7, 2021
@krwq krwq added this to Needs triage in Triage POD for Meta, Reflection, etc Feb 4, 2021
@joperezr joperezr moved this from Needs triage to Krzysztof's triage backlog in Triage POD for Meta, Reflection, etc Feb 9, 2021
@krwq
Copy link
Member

krwq commented Feb 16, 2021

@eerhardt are you planning to send a PR assuming this gets approved? I'll put this into 6.0 backlog for now.

@krwq krwq added this to the 6.0.0 milestone Feb 16, 2021
@krwq krwq moved this from Krzysztof's triage backlog to v-Next in Triage POD for Meta, Reflection, etc Feb 16, 2021
@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Feb 16, 2021
@eerhardt
Copy link
Member Author

@eerhardt are you planning to send a PR assuming this gets approved?

I can, yes.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 23, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 23, 2021

Video

  • Looks good as proposed
  • We should ensure that we address the other issues in the baseline file or add comments indicating why why are by-design.
namespace System.Reflection.Emit
{
    public sealed partial class EnumBuilder : TypeInfo
    {
        public Type? CreateType();
    }
}

@I-SER-I
Copy link
Contributor

I-SER-I commented Mar 6, 2021

@eerhardt And where should this be added if it exist in coreclr, but in corelib EnumBuilder is absent full...?

@stephentoub
Copy link
Member

It's already in the implementation:


It just needs to be added to the ref assembly (and tests added).

public System.Reflection.TypeInfo? CreateTypeInfo() { throw null; }

@eerhardt eerhardt self-assigned this Mar 8, 2021
eerhardt added a commit to eerhardt/runtime that referenced this issue Mar 8, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 8, 2021
Triage POD for Meta, Reflection, etc automation moved this from v-Next to Done Mar 10, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Emit
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants