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 System.Runtime.CompilerServices.IsExternalInit #37763

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jun 11, 2020

Resolves #34978

The up to date description on how we want to do this is here: #34978 (comment)

Looking at some of the other compiler-known attributes used in modreqs, it seems like the class should be 'static' as well.

I think there is no boilerplate unit test to add for this one because the static class doesn't contain any members.

@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.

/// This class should not be used by developers in source code.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public static class IsExternalInit
Copy link
Member

Choose a reason for hiding this comment

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

I just want to clarify... the issue lists the type as sealed:
#34978 (comment)
but here it's static. static rather than sealed was actually intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, after closer examination of other types used by the compiler in modreqs, we decided this should be a static class similar to IsVolatile

@RikkiGibson
Copy link
Contributor Author

@stephentoub is this ready for merge or is further review needed?

@RikkiGibson
Copy link
Contributor Author

Ping @jaredpar

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.

New API: InitOnlyAttribute
4 participants