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

Emit extension marker method as public #73994

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jun 13, 2024

Since we need the marker to roundtrip through reference assemblies, it should not be private.
Since the name is already unspeakable, I chose public accessibility.
Added a PROTOTYPE marker to record that the synthesized method is still not properly emitted in reference assemblies.

Corresponding spec update: dotnet/csharplang#8121
Relates to test plan #66722

@jcouv jcouv self-assigned this Jun 13, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 13, 2024
@jcouv jcouv marked this pull request as ready for review June 14, 2024 18:31
@jcouv jcouv requested a review from a team as a code owner June 14, 2024 18:31
@jcouv jcouv requested a review from AlekseyTs June 17, 2024 16:24
if (constructorHandle.Kind == HandleKind.MemberReference)
{
var typeRef = metadataReader.GetMemberReference((MemberReferenceHandle)constructorHandle).Parent;
return metadataReader.GetTypeReference((TypeReferenceHandle)typeRef).Name;
Copy link
Contributor

Choose a reason for hiding this comment

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

(TypeReferenceHandle)typeRef

Strictly speaking this cast isn't always valid, I think. From spec for MemberRef metadata table: "Class (an index into the MethodDef, ModuleRef,TypeDef, TypeRef, or TypeSpec
tables; more precisely, a MemberRefParent (§II.24.2.6) coded index)." I am not going to block on this because the change is for a test helper, it is an improvement and it is sufficient for the test scenarios we currently have.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@jcouv jcouv merged commit cf83dbe into dotnet:features/roles Jun 18, 2024
24 checks passed
@jcouv jcouv deleted the public-marker branch June 18, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers New Language Feature - Roles Roles untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants