Share code between source-generated and built-in marshallers #69043
Conversation
ghost
assigned 
...ystem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/Utf8StringMarshaller.cs
Outdated
Show resolved
Hide resolved
...ystem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/Utf8StringMarshaller.cs
Outdated
Show resolved
Hide resolved
|
@dotnet/interop-contrib What do you think about this? If we get an agreement that it is a good idea to share the implementation like this, I will reformat the other string marshallers to follow the same pattern. |
|
Looking only at the title, I am hesitant to do this. My reasoning is the unintentional impact this may have on the built-in system that we are stating is "static" minus bug fixes. I could easily see a scenario where we decided to change something minor in the new system and that flows into the built-in. I appreciate the desire to remove duplicate code but I think the two systems are separate for reasons of stability and innovative opportunity - these are competing interests in my mind. |
|
I believe that the fragility of the built-in system is in the advanced cases like reference type marshalling or COM interop. I would hope that string marshalling is simple-enough to not suffer from the fragility. We are making the string marshaller types public, so the innovation that can be applied to them will be fairly limited. One of my motivations for doing this is to reduce the engineering dept in NativeAOT. The built-in system in NativeAOT is using different set of helpers for interop. I wanted to use this as an opportunity to get everything on the same plan. |
I'd hope we have some latitude here, but your point is fair. I don't think this argument moves me much though.
Ugh. Well crap... this does change my perspective. I agree reconciling all of them would be a win. I will say the new entry points do seem to clutter the implementation and make something that is nicely standalone and easy to reason about more complicated. I guess I'm going to have to review this in detail. I think this is worth exploring. |
Haven't looked through all the changes (only skimmed the marshaller and stub helper in managed), but I am pro sharing. I consider the possibility of sharing like this to be one of the reasons we wanted the public string marshallers. |
| @@ -101,5 +103,26 @@ public void FreeNative() | |||
| if (_allocated != null) | |||
| Marshal.FreeCoTaskMem((IntPtr)_allocated); | |||
| } | |||
|
|
|||
| internal static string? ConvertToManaged(byte* value) | |||
There was a problem hiding this comment.
If these internal helpers are a sore spot, I can try to update the built-in marshalling to use the public methods and follow the regular pattern that depends on the marshaller instance everywhere.
There was a problem hiding this comment.
I think that is the sore spot for me, at least that is what I initially noticed. If we could get rid of them I think I'd be willing to let my other concerns go.
There was a problem hiding this comment.
Ok, I have reverted the internal helpers and change the IL generation to target the public methods. How does it look?
jkotas
force-pushed
the
unify-marshallers
branch
3 times, most recently
from
4941bb4
to
06b6514
Compare
No description provided.