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.

Sign up for GitHub

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

Jump to bottom

[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. #11560

Merged
merged 1 commit into from
May 17, 2021
Merged

[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. #11560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions src/CoreGraphics/CGPDFOperatorTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,35 @@ protected virtual void Dispose (bool disposing)

public IntPtr Handle { get; private set; }

// We need this P/Invoke for legacy AOT scenarios (since we have public API taking a 'Action<IntPtr, IntPtr>', and with this particular native function we can't wrap the delegate)
// Unfortunately CoreCLR doesn't support generic Action delegates in P/Invokes: https://github.com/dotnet/runtime/issues/32963
[DllImport (Constants.CoreGraphicsLibrary)]
extern static void CGPDFOperatorTableSetCallback (/* CGPDFOperatorTableRef */ IntPtr table, /* const char */ string name, /* CGPDFOperatorCallback */ Action<IntPtr,IntPtr> callback);

#if NET
// This signature requires C# 9 (so .NET only).
// The good part about this signature is that it enforces at compile time that 'callback' is callable from native code in a FullAOT scenario.
// The bad part is that it's unsafe code (and callers must be in unsafe mode as well).
[DllImport (Constants.CoreGraphicsLibrary)]
unsafe extern static void CGPDFOperatorTableSetCallback (/* CGPDFOperatorTableRef */ IntPtr table, /* const char */ string name, /* CGPDFOperatorCallback */ delegate* unmanaged<IntPtr, IntPtr, void> callback);
#endif

#if MONOMAC
// This signature can work everywhere, but we can't enforce at compile time that 'callback' is a delegate to a static function (which is required for FullAOT scenarios),
// so limit it to non-FullAOT platforms (macOS)
[DllImport (Constants.CoreGraphicsLibrary)]
extern static void CGPDFOperatorTableSetCallback (/* CGPDFOperatorTableRef */ IntPtr table, /* const char */ string name, /* CGPDFOperatorCallback */ CGPDFOperatorCallback callback);

// this won't work with AOT since the callback must be decorated with [MonoPInvokeCallback]
public void SetCallback (string name, Action<CGPDFScanner,object> callback)
{
if (name == null)
throw new ArgumentNullException ("name");

if (callback == null)
CGPDFOperatorTableSetCallback (Handle, name, null);
CGPDFOperatorTableSetCallback (Handle, name, (CGPDFOperatorCallback) null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah I don't recall seeing this before haha. A type cast for null! What does this do?

Copy link
Member

Choose a reason for hiding this comment

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

@tj-devel709 imaging you are a compiler and you see:

public void CGPDFOperatorTableSetCallback (IntPtr handel, String name, CGPDFOperatorCallback callback) {...}
public void CGPDFOperatorTableSetCallback (IntPtr handel, String name, OtherObject obj) {..}

Later in the code I have:

CGPDFOperatorTableSetCallback (Handle, name, null);

Which of the above overloads do you call??

And if you see:

CGPDFOperatorTableSetCallback (Handle, name, (CGPDFOperatorCallback) null);

AFAIK you can also do:

CGPDFOperatorTableSetCallback (Handle, name, default(CGPDFOperatorCallback));

which helps the compiler too without a cast.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thank you for explaining!

else
CGPDFOperatorTableSetCallback (Handle, name, new Action<IntPtr, IntPtr> (delegate (IntPtr reserved, IntPtr gchandle) {
CGPDFOperatorTableSetCallback (Handle, name, new CGPDFOperatorCallback (delegate (IntPtr reserved, IntPtr gchandle) {
// we could do 'new CGPDFScanner (reserved, true)' but we would not get `UserInfo` (managed state) back
// we could GCHandle `userInfo` but that would (even more) diverge both code bases :(
var scanner = GetScannerFromInfo (gchandle);
Expand All @@ -94,7 +109,11 @@ public void SetCallback (string name, Action<CGPDFScanner,object> callback)

[Advice ("Use the nicer SetCallback(string,Action<CGPDFScanner,object>) API when possible.")]
#endif

// this API is ugly - but I do not see a better way with the AOT limitation
#if NET && !MONOMAC
[Obsolete ("Use the overload that takes a function pointer ('delegate*<IntPtr,IntPtr,void>') instead.")]
#endif
public void SetCallback (string name, Action<IntPtr,IntPtr> callback)
{
if (name == null)
Expand All @@ -103,6 +122,17 @@ public void SetCallback (string name, Action<IntPtr,IntPtr> callback)
CGPDFOperatorTableSetCallback (Handle, name, callback);
}

#if NET
// this signature requires C# 9 and unsafe code
public unsafe void SetCallback (string name, delegate* unmanaged<IntPtr, IntPtr, void> callback)
{
if (name == null)
throw new ArgumentNullException (nameof (name));

CGPDFOperatorTableSetCallback (Handle, name, callback);
}
#endif

static public CGPDFScanner GetScannerFromInfo (IntPtr gchandle)
{
return GCHandle.FromIntPtr (gchandle).Target as CGPDFScanner;
Expand Down
17 changes: 17 additions & 0 deletions tests/monotouch-test/CoreGraphics/PDFScannerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
//

using System;
using System.Runtime.InteropServices;

using Foundation;
using CoreGraphics;
using ObjCRuntime;
Expand All @@ -22,7 +24,11 @@ public class PDFScannerTest {
int bt_count;
int do_checks;

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (Action<IntPtr,IntPtr>))]
#endif
static void BT (IntPtr reserved, IntPtr info)
{
// sadly the parameters are always identical and we can't know the operator name
Expand All @@ -35,7 +41,11 @@ static void BT (IntPtr reserved, IntPtr info)
(scanner.UserInfo as PDFScannerTest).bt_count++;
}

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (Action<IntPtr,IntPtr>))]
#endif
static void Do (IntPtr reserved, IntPtr info)
{
// sadly the parameters are always identical and we can't know the operator name
Expand Down Expand Up @@ -97,6 +107,13 @@ public void Tamarin ()
table.SetCallback ("Do", delegate (CGPDFScanner scanner) {
// ... drill down to the image
});
#elif NET
unsafe {
// BT == new paragraph
table.SetCallback ("BT", &BT);
// Do == the image is inside it
table.SetCallback ("Do", &Do);
}
#else
// BT == new paragraph
table.SetCallback ("BT", BT);
Expand Down