-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add an intermediate class, MonoMarshaler, to hide Mono bugs #2521
Conversation
allocated_intptrs_.Remove(native_data); | ||
} | ||
} | ||
if (actual_native_data != IntPtr.Zero) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document, either here or on allocated_intptrs_
, what we are doing (the comment at the top of the class documents what the issue is, but the solution is nontrivial and deserves a comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ksp_plugin_adapter/utf8_marshaler.cs
Outdated
public static ICustomMarshaler GetInstance(string s) { | ||
UnityEngine.Debug.LogError("UnownedUTF8Marshaler.GetInstance"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the traces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, done.
// marshalers must implement a static method called |GetInstance| that accepts | ||
// a |String| as a parameter and has a return type of |ICustomMarshaler|, | ||
// see https://goo.gl/wwmBTa. | ||
// A marshaler for UTF-8 strings whose ownership is not taken from C++. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Not taken from C++" is a property that only makes sense for results/out parameters: for in and in out parameters there is no C++ ownership to take.
Further, the marshaler name itself is misleading: for in and in out parameters, the string ownership is C#-side—and since the marshaler is doing the construction and destruction, it owns the string.
This really is a NoTransferOfOwnershipUTF8Marshaler
: either the string may be owned by C++ or by C#, but it stays that way. That seems like too many words for an identifier; maybe UncededUTF8Marshaler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The verb "cede" in this context reads weird. Let's discuss the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ksp_plugin_adapter/utf8_marshaler.cs
Outdated
internal class OutOwnedUTF8Marshaler : OutUTF8Marshaler { | ||
// A marshaler for UTF-8 strings whose ownership is taken from C++. Useful for | ||
// out parameters and returned values. | ||
internal class OwnedUTF8Marshaler : UnownedUTF8Marshaler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By symmetry, CededUTF8Marshaler
.
Note that deriving from the other one seems a bit weird now.
If we wanted to use it for in parameters, it would have to allocate via an interface function. We have no use for that at the moment, so it should probably Log.Fatal
if we accidentally use it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Granted, inheritance is a bit odd, but then composition doesn't work so well for classes that cannot have nonstatic members.
I don't understand the part about allocating via an interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
retest this please |
Rebase all the marshalers on this class, and unify
in
andout
if needed.First step towards addressing #2339.