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

Enhance the bound generator concept to remove duplicate parameters and make it more pervasive throughout the system #105597

Merged
merged 13 commits into from
Aug 15, 2024

Conversation

jkoritzinsky
Copy link
Member

This reduces the number of places where we need to manually pass around a marshalling generator and its matching TypePositionInfo.

This PR also simplifies some portions of the JS generators.

Addresses #89033

Moving the generators to also store the stub code context would end up requiring us to rebind in the majority of cases, increasing GC load and not actually improving maintainability much.

@pavelsavara
Copy link
Member

It would be great to introduce some test which would assert the explicit shape of the output code, so that we feel safer doing refactorings.

We had situation that the previous refactoring introduced duplicate statement in the generated code and we have not noticed for quite long time. more details

@pavelsavara pavelsavara requested a review from maraf July 29, 2024 09:21
@jkoritzinsky
Copy link
Member Author

I've added a test that validates the exact code output (and fixed a case of non-determinism in the JS generators in the process)

@pavelsavara
Copy link
Member

I've added a test that validates the exact code output (and fixed a case of non-determinism in the JS generators in the process)

thank you!

Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

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

Looks good to me, but since this is a big change and just a refactor, we should probably wait until after we snap for 9 to merge

@jkoritzinsky
Copy link
Member Author

I'll move this and the corresponding issue to .NET 10.

@jkoritzinsky jkoritzinsky added this to the 10.0.0 milestone Aug 12, 2024
@jkoritzinsky jkoritzinsky merged commit 7fd8724 into dotnet:main Aug 15, 2024
92 of 94 checks passed
@jkoritzinsky jkoritzinsky deleted the bound-marshaller branch August 15, 2024 03:55
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
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
Status: Done
4 participants