Skip to content

Add support for UnsafeAccessorKind.FieldOffset (#45152) - #93946

Closed
xoofx wants to merge 1 commit into
dotnet:mainfrom
xoofx:unsafe-accessor-field-offset
Closed

xoofx wants to merge 1 commit into
dotnet:mainfrom
xoofx:unsafe-accessor-field-offset

Conversation

@xoofx

@xoofx xoofx commented Oct 24, 2023

Copy link
Copy Markdown
Member

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 community-contribution Indicates that the PR has been added by a community member area-System.Runtime.CompilerServices new-api-needs-documentation labels Oct 24, 2023
@ghost

ghost commented Oct 24, 2023

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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
Copy Markdown
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());

@hamarb123 hamarb123 Oct 26, 2023 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

ghost commented Nov 25, 2023

Copy link
Copy Markdown

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