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.

Sign up for GitHub

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

Jump to bottom

Add Enum generic overloads #33589

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Add Enum generic overloads #33589

merged 1 commit into from
Jul 13, 2020

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Mar 14, 2020

Fixes #2364
Contributes to #20008
Maybe fixes #18063

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@hughbe hughbe force-pushed the Enum-Generic-tests branch from a95f38d to 571ef84 Compare March 14, 2020 16:43
@hughbe hughbe force-pushed the Enum-Generic-tests branch from 571ef84 to c814ad2 Compare March 14, 2020 17:58
stephentoub
stephentoub reviewed Mar 16, 2020
View reviewed changes
src/libraries/Common/tests/CoreFx.Private.TestUtilities/System/PlatformDetection.cs Outdated
#else
public static bool IsReflectionEmitSupported = true;
public static bool IsReflectionEmitSupported => true;
Copy link
Member

Choose a reason for hiding this comment

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

Why did these change as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I need to use ConditionalFact, which only supports properties and not fields

stephentoub
stephentoub reviewed Mar 16, 2020
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Enum.cs Outdated
=> (TEnum[])GetValues(typeof(TEnum));

public static bool IsDefined<TEnum>(TEnum value) where TEnum : struct, Enum
=> IsDefined(typeof(TEnum), value);
Copy link
Member

@stephentoub stephentoub Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What is the benefit of this method if it's just going to box the argument? Just being able to write Enum.IsDefined(value) instead of Enum.IsDefined(typeof(MyEnum), value)?

Copy link
Member

@jkotas jkotas Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This can be certainly implemented more efficiently. It just needs to do this:

runtime/src/libraries/System.Private.CoreLib/src/System/RuntimeType.cs

Lines 267 to 270 in 25ea6c4

ulong[] ulValues = Enum.InternalGetValues(this);
ulong ulValue = Enum.ToUInt64(value);
return Array.BinarySearch(ulValues, ulValue) >= 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true. I will investigate this tomorrow. Just rebasing now to make sure it all builds

Copy link

Choose a reason for hiding this comment

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

@stephentoub One benefit would be that the value and type cannot be accidentally mismatched.

@hughbe hughbe force-pushed the Enum-Generic-tests branch 4 times, most recently from 18d04fb to fe1ba0a Compare March 21, 2020 09:19
@jaredpar jaredpar mentioned this pull request Mar 21, 2020
Closed
@hughbe hughbe force-pushed the Enum-Generic-tests branch from fe1ba0a to b630618 Compare May 3, 2020 10:50
@hughbe hughbe force-pushed the Enum-Generic-tests branch from b630618 to 284d3a3 Compare June 15, 2020 15:57
@steveharter
Copy link
Member

@hughbe the Preview 7 cutoff is quickly approaching (July 15th). Please try to address feedback before then. Thanks

@hughbe
Copy link
Contributor Author

hughbe commented Jul 9, 2020

Okay. Working on this now, sorry for the delay!

@hughbe hughbe force-pushed the Enum-Generic-tests branch from 284d3a3 to 75af538 Compare July 9, 2020 10:29
@hughbe
Copy link
Contributor Author

hughbe commented Jul 9, 2020

I moved some stuff around because it was shared between mono/CoreCLR, and optimized the implementation of IsDefined

@hughbe hughbe force-pushed the Enum-Generic-tests branch from 75af538 to 13eecb5 Compare July 9, 2020 13:29
@hughbe
Copy link
Contributor Author

hughbe commented Jul 9, 2020 •
edited
Loading

Jan, could you also check if there are any worthwile opportunities to optimise the generic functions. I could avoid a few checks like IsEnum on RuntimeType in things like GetValues etc, but didn't think it was worth the code duplication. Alternatively I could implement RuntimeType APIs in terms of Enum generic functions? Actually this would not be possible i think

@jaredpar jaredpar mentioned this pull request Jul 9, 2020
Closed
@jkotas
Copy link
Member

jkotas commented Jul 9, 2020

Agree. The reason for adding these APIs is syntax sugar, not performance.

@hughbe hughbe force-pushed the Enum-Generic-tests branch from 713bd57 to 730c01e Compare July 10, 2020 10:21
@akoeplinger akoeplinger requested a review from jkotas July 13, 2020 15:34
jkotas
jkotas approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 7b2e585 into dotnet:master Jul 13, 2020
@hughbe hughbe deleted the Enum-Generic-tests branch July 13, 2020 16:46
@jkotas jkotas mentioned this pull request Jul 21, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GSPP GSPP GSPP left review comments

@stephentoub stephentoub stephentoub left review comments

@jkotas jkotas jkotas approved these changes

@akoeplinger akoeplinger akoeplinger approved these changes

@EgorBo EgorBo Awaiting requested review from EgorBo

@marek-safar marek-safar Awaiting requested review from marek-safar

@steveisok steveisok Awaiting requested review from steveisok

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-System.Runtime new-api-needs-documentation
Projects
None yet
Milestone
No milestone
7 participants
@hughbe @Dotnet-GitSync-Bot @steveharter @jkotas @akoeplinger @stephentoub @GSPP