Refactor our linear collection marshalling implementation #86519
Conversation
Refactor the elements marshalling logic to split out space allocation and source/destination span construction from the elements marshalling logic. Pull out the element marshalling logic to a common base class and separate it out from the actual marshalling strategies. This refactoring is required to provide customization points to correctly fix dotnet#85795. I've validated that this change is a zero-diff change on all of the generated code for the LibraryImportGenerator integration tests.
|
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsRefactor the elements marshalling logic to split out space allocation and source/destination span construction from the elements marshalling logic. Pull out the element marshalling logic to a common base class and separate it out from the actual marshalling strategies. This refactoring is required to provide customization points to correctly fix #85795. I've validated that this change is a zero-diff change on all of the generated code for the LibraryImportGenerator integration tests.
|
ghost
assigned 
...opServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatefulMarshallingStrategy.cs
Outdated
Show resolved
Hide resolved
...opServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatefulMarshallingStrategy.cs
Outdated
Show resolved
Hide resolved
…erop.SourceGeneration/Marshalling/StatefulMarshallingStrategy.cs
...opServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatefulMarshallingStrategy.cs
Outdated
Show resolved
Hide resolved
...pServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatelessMarshallingStrategy.cs
Outdated
Show resolved
Hide resolved
...pServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatelessMarshallingStrategy.cs
Outdated
Show resolved
Hide resolved
...pServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatelessMarshallingStrategy.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| internal sealed class StatelessLinearCollectionBlittableElementsMarshalling : BlittableElementsMarshalling, ICustomTypeMarshallingStrategy | ||
| internal sealed class StatelessLinearCollectionSpaceAllocator : ICustomTypeMarshallingStrategy |
There was a problem hiding this comment.
Is this type ever meant to be used as an ICustomTypeMarshallingStrategy on its own? It looks like it's pretty coupled to the StatelessLinearCollectionMarshalling. Probably not in this PR, but for types like this where it has all the methods of ICustomTypeMarshallingStrategy, but is always supposed to be an "inner marshaller" and isn't meant to be its own the top level ICustomTypeMarshallingStrategy, maybe we should make a different type to make that more clear.
e.g. We could move all the methods from ICustomTypeMarshallingStrategy to a base interface "IMarshallingStatementsGenerator" and have ICustomTypeMarshallingStrategy and ISpaceAllocator both "Marker interfaces" that inherit from it. It might add a little metadata, but I was a bit confused about these scenarios when starting in the codebase.
There was a problem hiding this comment.
I like that idea. Once this is in, I'll take a look at doing something like that to make it clearer.
...me.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/ElementsMarshalling.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
|
All failures are known issues. Merging this in. |
Refactor the elements marshalling logic to split out space allocation and source/destination span construction from the elements marshalling logic. Pull out the element marshalling logic to a common base class and separate it out from the actual marshalling strategies.
This refactoring is required to provide customization points to author the fix for #85795.
I've validated that this change is a zero-diff change on all of the generated code for the LibraryImportGenerator integration tests.