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

[CoreCLR/NativeAOT] UnsafeAccessorAttribute non-generic support #86932

Merged
merged 55 commits into from
Jun 15, 2023

Conversation

AaronRobinsonMSFT
Copy link
Member

Contributes to #86161

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

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented May 31, 2023

/cc @lambdageek @fanyang-mono @roji

src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/dynamicmethod.h Outdated Show resolved Hide resolved
src/coreclr/vm/dynamicmethod.cpp Outdated Show resolved Hide resolved
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review May 31, 2023 20:51
/// <summary>
/// Provide access to a Method.
/// </summary>
Method,
Copy link
Member

Choose a reason for hiding this comment

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

Is this any method or just instance method? The naming is strange if it's the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is for an instance method. That is what API decided upon. I can go back and ask if you think InstanceMethod is more appropriate?

Copy link
Contributor

@IDisposable IDisposable left a comment

Choose a reason for hiding this comment

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

So reminiscent of the old days ...

src/coreclr/vm/dynamicmethod.h Outdated Show resolved Hide resolved
src/coreclr/vm/jitinterface.cpp Show resolved Hide resolved
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

path has evolved to require exceptions to
propagate for generation of typeload exception
stub.
src/coreclr/vm/siginfo.hpp Outdated Show resolved Hide resolved
src/coreclr/vm/siginfo.cpp Outdated Show resolved Hide resolved
Remove unmanage resource string and move it to SPCL
Share the AmbiguousMatchException message between
  NativeAOT and CoreCLR.
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your work on this feature!

@AaronRobinsonMSFT
Copy link
Member Author

@MichalStrehovsky Can you take another look at the NativeAOT portions? I'd like a sign off from you too with respect to how this is implemented there.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 5649739 into dotnet:main Jun 15, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_86161 branch June 15, 2023 13:59
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title [CoreCLR] UnsafeAccessorAttribute non-generic support [CoreCLR/NativeAOT] UnsafeAccessorAttribute non-generic support Jun 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 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.

8 participants