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 support for UnsafeAccessorKind.FieldOffset (#45152) #93946

Closed
wants to merge 1 commit into from

Conversation

xoofx
Copy link
Member

@xoofx xoofx commented Oct 24, 2023

Draft PR to add support for UnsafeAccessorKind.FieldOffset.

This PR is for now just a proof of concept of the suggestion of implementing field offset computation via UnsafeAccessorKind.FieldOffset as suggested here

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 24, 2023
@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
Copy link

ghost commented Oct 24, 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

Draft PR to add support for UnsafeAccessorKind.FieldOffset.

This PR is for now just a proof of concept of the suggestion of implementing field offset computation via UnsafeAccessorKind.FieldOffset as suggested here

Author: xoofx
Assignees: -
Labels:

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

Milestone: -

@AaronRobinsonMSFT
Copy link
Member

@xoofx Do you want to file an API proposal for this? That would be the next step.

Console.WriteLine($"Running {nameof(Verify_AccessFieldOffsetClass)}");

// Offset = method table pointer + first string pointer
Assert.Equal(IntPtr.Size + IntPtr.Size, GetPrivateFieldOffset());
Copy link
Contributor

@hamarb123 hamarb123 Oct 26, 2023

Choose a reason for hiding this comment

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

This should just be IntPtr.Size I think, since you cannot legitimately obtain a ref before the first field iirc.
Presumably the GetRawData API will give the offset of the 0th field.
Also, should we have tests for structs and for ref fields?

(edit: I'm editing this because my edge crashed after posting my comments and undid all my changes & deleted a comment?!)

Assert.Equal(IntPtr.Size + IntPtr.Size, GetPrivateFieldOffset());

[UnsafeAccessor(UnsafeAccessorKind.FieldOffset, Name=UserDataClass.FieldSecondName)]
extern static nint GetPrivateFieldOffset(UserDataClass? d = null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what others' opinions on this are, but I like to be explicit with these sorts of APIs - I'd like it if we could also specify a second parameter which is the type of the field - it would first check for fields with same type (including modifiers), and then same type (ignoring modifiers) (same as other checks for the UnsafeAccessors iirc).

@ghost
Copy link

ghost commented Nov 25, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 26, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants