-
Notifications
You must be signed in to change notification settings - Fork 507
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
[.NET/CoreGraphics] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 #15906
[.NET/CoreGraphics] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 #15906
Conversation
This comment has been minimized.
This comment has been minimized.
src/CoreGraphics/CGFunction.cs
Outdated
|
||
#if NET | ||
[StructLayout (LayoutKind.Sequential)] | ||
unsafe struct CGFunctionCallbacks { | ||
public /* unsigned int */ uint version; | ||
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate; | ||
public delegate* unmanaged<IntPtr, void> release; | ||
} | ||
#else | ||
[StructLayout (LayoutKind.Sequential)] | ||
struct CGFunctionCallbacks { | ||
public /* unsigned int */ uint version; | ||
public CGFunctionEvaluateCallback? evaluate; | ||
public CGFunctionReleaseCallback? release; | ||
} | ||
#endif |
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.
Since the struct is the same and we might add more data in it, why not doing:
#if NET | |
[StructLayout (LayoutKind.Sequential)] | |
unsafe struct CGFunctionCallbacks { | |
public /* unsigned int */ uint version; | |
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate; | |
public delegate* unmanaged<IntPtr, void> release; | |
} | |
#else | |
[StructLayout (LayoutKind.Sequential)] | |
struct CGFunctionCallbacks { | |
public /* unsigned int */ uint version; | |
public CGFunctionEvaluateCallback? evaluate; | |
public CGFunctionReleaseCallback? release; | |
} | |
#endif | |
[StructLayout (LayoutKind.Sequential)] | |
struct CGFunctionCallbacks { | |
public /* unsigned int */ uint version; | |
#if NET | |
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate; | |
public delegate* unmanaged<IntPtr, void> release; | |
#else | |
public CGFunctionEvaluateCallback? evaluate; | |
public CGFunctionReleaseCallback? release; | |
#endif | |
} |
That way if we need to add more stuff, we won't have to write it twice.
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.
I considered that, but since the struct needs to be marked unsafe
, this would start to get into the ridiculous range:
[StructLayout (LayoutKind.Sequential)]
#if NET
unsafe struct CGFunctionCallbacks {
#else
struct CGFunctionCallbacks {
#endif
public /* unsigned int */ uint version;
#if NET
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
public delegate* unmanaged<IntPtr, void> release;
#else
public CGFunctionEvaluateCallback? evaluate;
public CGFunctionReleaseCallback? release;
#endif
}
At this point every element is different except for the attribute and the uint.
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.
It doesn't work if you mark just the fields as unsafe?
public unsafe delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
public unsafe delegate* unmanaged<IntPtr, void> release;
@@ -136,17 +150,24 @@ public unsafe CGFunction (nfloat []? domain, nfloat []? range, CGFunctionEvaluat | |||
var handle = CGFunctionCreate (GCHandle.ToIntPtr (gch), domain is not null ? domain.Length / 2 : 0, domain, range is not null ? range.Length / 2 : 0, range, ref cbacks); | |||
InitializeHandle (handle); | |||
} | |||
|
|||
#if NET |
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 [MonoPInvokeCallback (typeof (CGFunctionReleaseCallback))] was just being added in those platforms that are not macOS, now with the #If NET the UnmanagedCallersOnly is added in all platforms including macOS, is this intended?
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.
Yes. It's intentional. My understanding is that macOS doesn't need the [MonoPInvoke]
attribute, but other platforms do. For .NET 7, everybody gets [UnmanagedCallersOnly]
#endif | ||
static void ReleaseCallback (IntPtr info) | ||
{ | ||
GCHandle.FromIntPtr (info).Free (); | ||
} | ||
|
||
#if NET | ||
[UnmanagedCallersOnly] | ||
#else |
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.
Same question as the previous one.
|
||
#if NET | ||
[UnmanagedCallersOnly] | ||
#else | ||
#if !MONOMAC |
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.
Same.
src/CoreGraphics/CGPattern.cs
Outdated
#if NET | ||
[StructLayout (LayoutKind.Sequential)] | ||
unsafe struct CGPatternCallbacks { | ||
internal /* unsigned int */ uint version; | ||
internal delegate* unmanaged<IntPtr, IntPtr, void> draw; | ||
internal delegate* unmanaged<IntPtr, void> release; | ||
} | ||
#else | ||
[StructLayout (LayoutKind.Sequential)] | ||
struct CGPatternCallbacks { | ||
internal /* unsigned int */ uint version; | ||
internal DrawPatternCallback draw; | ||
internal ReleaseInfoCallback release; | ||
} | ||
#endif |
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.
Same comment about the struct.
#if NET | |
[StructLayout (LayoutKind.Sequential)] | |
unsafe struct CGPatternCallbacks { | |
internal /* unsigned int */ uint version; | |
internal delegate* unmanaged<IntPtr, IntPtr, void> draw; | |
internal delegate* unmanaged<IntPtr, void> release; | |
} | |
#else | |
[StructLayout (LayoutKind.Sequential)] | |
struct CGPatternCallbacks { | |
internal /* unsigned int */ uint version; | |
internal DrawPatternCallback draw; | |
internal ReleaseInfoCallback release; | |
} | |
#endif | |
[StructLayout (LayoutKind.Sequential)] | |
struct CGPatternCallbacks { | |
internal /* unsigned int */ uint version; | |
#if NET | |
internal delegate* unmanaged<IntPtr, IntPtr, void> draw; | |
internal delegate* unmanaged<IntPtr, void> release; | |
#else | |
internal DrawPatternCallback draw; | |
internal ReleaseInfoCallback release; | |
} | |
#endif |
@@ -111,8 +134,12 @@ public CGPattern (CGRect bounds, CGAffineTransform matrix, nfloat xStep, nfloat | |||
Handle = CGPatternCreate (GCHandle.ToIntPtr (gch), bounds, matrix, xStep, yStep, tiling, isColored, ref callbacks); | |||
} | |||
|
|||
#if NET | |||
[UnmanagedCallersOnly] | |||
#else |
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.
Same, it adds macOS when it did not in the past.
@@ -123,8 +150,12 @@ static void DrawCallback (IntPtr voidptr, IntPtr cgcontextptr) | |||
} | |||
} | |||
|
|||
#if NET | |||
[UnmanagedCallersOnly] | |||
#else | |||
#if !MONOMAC |
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.
Same.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/CoreGraphics/CGDataProvider.cs
Outdated
[DllImport (Constants.CoreGraphicsLibrary)] | ||
extern static IntPtr CGDataProviderCreateWithData (/* void* */ IntPtr info, /* const void* */ IntPtr data, /* size_t */ nint size, /* CGDataProviderReleaseDataCallback */ CGDataProviderReleaseDataCallback releaseData); | ||
#endif | ||
|
||
delegate void CGDataProviderReleaseDataCallback (IntPtr info, IntPtr data, nint size); |
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.
Is this delegate still needed in NET?
src/CoreGraphics/CGFunction.cs
Outdated
|
||
#if NET | ||
[StructLayout (LayoutKind.Sequential)] | ||
unsafe struct CGFunctionCallbacks { | ||
public /* unsigned int */ uint version; | ||
public delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate; | ||
public delegate* unmanaged<IntPtr, void> release; | ||
} | ||
#else | ||
[StructLayout (LayoutKind.Sequential)] | ||
struct CGFunctionCallbacks { | ||
public /* unsigned int */ uint version; | ||
public CGFunctionEvaluateCallback? evaluate; | ||
public CGFunctionReleaseCallback? release; | ||
} | ||
#endif |
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.
It doesn't work if you mark just the fields as unsafe?
public unsafe delegate* unmanaged<IntPtr, nfloat*, nfloat*, void> evaluate;
public unsafe delegate* unmanaged<IntPtr, void> release;
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 0 tests crashed, 2 tests failed, 151 tests passed. Failures❌ monotouch tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Completed the work for CoreGraphics.
One exception is PDFArray.cs which uses
SetupBlockUnsafe
(noted in the issue)