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

Add an intermediate class, MonoMarshaler, to hide Mono bugs #2521

Merged
merged 12 commits into from
Apr 7, 2020

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Apr 6, 2020

Rebase all the marshalers on this class, and unify in and out if needed.

First step towards addressing #2339.

allocated_intptrs_.Remove(native_data);
}
}
if (actual_native_data != IntPtr.Zero) {
Copy link
Member

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

public static ICustomMarshaler GetInstance(string s) {
UnityEngine.Debug.LogError("UnownedUTF8Marshaler.GetInstance");
Copy link
Member

Choose a reason for hiding this comment

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

Remove the traces.

Copy link
Member Author

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++.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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 {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

@eggrobin eggrobin added the LGTM label Apr 7, 2020
@pleroy
Copy link
Member Author

pleroy commented Apr 7, 2020

retest this please

@pleroy pleroy merged commit 040ca5b into mockingbirdnest:master Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants