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

Update all internal marshallers to the new model #71470

Merged
merged 6 commits into from
Jul 7, 2022

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jun 30, 2022

Once this PR is in, we have no usages of the v1 model that aren't already public API or approved to be a public API.

@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Jun 30, 2022
@ghost ghost assigned jkoritzinsky Jun 30, 2022
@ghost
Copy link

ghost commented Jun 30, 2022

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

Issue Details

Gets us closer to being able to remove the v1 model as soon as we get the v2 model approved by API review.

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: -

@jkoritzinsky jkoritzinsky changed the title Update all internal marshallers that don't require pinning support to the new model Update all internal marshallers to the new model Jul 5, 2022
@jkoritzinsky
Copy link
Member Author

This PR is ready for review again.

Comment on lines +92 to +93
[SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "This method is part of the marshaller shape and is required to be an instance method.")]
public void Free() { }
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's how the empty Free methods look when they're required on the stateful shape. Just wanted to raise it for us to look at.

Might be worth investigating introducing a DiagnosticSuppressor to suppress this diagnostic in these cases as following the code fix would break the marshaller (and the diagnostic is configured to be an error in dotnet/runtime, so it needs to be manually suppressed here today)

Copy link
Member

Choose a reason for hiding this comment

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

Oof, yeah, that is unfortunate. We should look into a suppressor - can you file an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@jkoritzinsky jkoritzinsky merged commit 0c631ab into dotnet:main Jul 7, 2022
@jkoritzinsky jkoritzinsky deleted the custom-marshallers-v2-usage branch July 7, 2022 23:26
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants