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

Expose the RequiresLocationAttribute #89870

Merged
merged 2 commits into from
Aug 11, 2023
Merged

Conversation

tannergooding
Copy link
Member

This resolves #85910. It is necessary to avoid potential downstream issues with the use of the attribute as a modreq in virtual members and function pointers.

@dotnet-issue-labeler
Copy link

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

@ghost ghost assigned tannergooding Aug 2, 2023
@ghost
Copy link

ghost commented Aug 2, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85910. It is necessary to avoid potential downstream issues with the use of the attribute as a modreq in virtual members and function pointers.

Author: tannergooding
Assignees: -
Labels:

area-System.Runtime.CompilerServices, new-api-needs-documentation

Milestone: -

@tannergooding
Copy link
Member Author

Marked as draft while this waits for final confirmation from @jaredpar, @jjonescz, @stephentoub

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

@stephentoub
Copy link
Member

Given our offline discussion, I think we should add this now. But I'd like for @jaredpar and/or @jjonescz to confirm we don't foresee any reason to change the attribute before C# 12 ships.

@jaredpar
Copy link
Member

jaredpar commented Aug 2, 2023

But I'd like for @jaredpar and/or @jjonescz to confirm we don't foresee any reason to change the attribute before C# 12 ships.

I do not forsee any changes. There is a C# LDM meeting on ref readonly next Wednesday (Aug 9th). That is just a standard post merge follow up meeting to discuss implementation issues. Don't expect any changes but if they were too happen it would be there. After that it, and even at this point, would take something really significant for us to change the attribute.

@jjonescz
Copy link
Member

jjonescz commented Aug 3, 2023

It is necessary to avoid potential downstream issues with the use of the attribute as a modreq in virtual members and function pointers.

Btw, the attribute isn't used as modreq in virtual methods, the existing InAttribute is used there. RequiresLocationAttribute is used only as modopt in function pointers.

@stephentoub stephentoub marked this pull request as ready for review August 3, 2023 11:08
@stephentoub stephentoub merged commit 678fd6a into dotnet:main Aug 11, 2023
166 of 168 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
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.

[API Proposal]: RequiresLocationAttribute (supporting ref readonly parameters)
5 participants