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

Implement collection marshaller spec #1197

Merged

Conversation

jkoritzinsky
Copy link
Member

Implement the spec in SpanMarshallers.md for collections.

Move arrays over to using the collection marshallers internally.

Refactor attribute inspection to more easily support holistic analysis of marshalling attributes (see what's used, what isn't, and what provides conflicting info).

This PR does not add built-in support for marshalling spans. That's planned for a future PR (though the implementation will likely follow the ArrayMarshaller in Ancillary.Interop very closely).

… Implement an array marshaller in generator that wraps the collection marshaller model to support the special pinning and [Out] semantics required for arrays.
Add special marshaller for arrays of pointers to ensure they are still supported.
…dditional constructor to specify native element size for the unmarshal-only case.
…tter holistic analysis of all use-site marshalling attributes.
…te usage validation for ElementIndirectionLevel.
…t the custom type and collection marshalling generation in a less confusing albeit more verbose way.
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

Overall, I like this a lot more. I am struggling to reconcile the nested collection logic's cost with the user benefit. I am thinking of the servicing burden of owning such a complex piece of logic for the very few users in practice. I wonder if there is some way we could excise that logic and ensure we have the capability for users do to it themselves if it becomes interesting.

@jkoritzinsky
Copy link
Member Author

This PR doesn't have any of the work to support nested collections. That's in a separate local branch I haven't pushed up. This PR just has the support for the collections spec as written. It doesn't support collection elements with complex marshallers (marshallers with a Value property). That's also later work.

@jkoritzinsky
Copy link
Member Author

Well, it does have support for the ElementIndirectionLevel validation, which is a prereq to nested collections support.

@jkoritzinsky
Copy link
Member Author

If we wanted to, we could just block any ElementIndirectionLevel values above 1. Though we'd still need the same validation for levels 0 and 1, so it wouldn't necessarily be much simpler.

@AaronRobinsonMSFT
Copy link
Member

Well, it does have support for the ElementIndirectionLevel validation, which is a prereq to nested collections support.

Yes, that is the logic I was referring to.

If we wanted to, we could just block any ElementIndirectionLevel values above 1. Though we'd still need the same validation for levels 0 and 1, so it wouldn't necessarily be much simpler.

Okay.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

LGTM.

@jkoritzinsky jkoritzinsky merged commit b3a0ead into dotnet:feature/DllImportGenerator Jun 9, 2021
@jkoritzinsky jkoritzinsky deleted the collections branch June 9, 2021 17:41
jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this pull request Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DllImportGenerator Source Generated stubs for P/Invokes in C#
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants