Skip to content

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

Merged
rolfbjarne merged 1 commit into
dotnet:mainfrom
rolfbjarne:coreclr-coregraphics-cgpdfoperatortable
May 17, 2021
Merged

[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate.#11560
rolfbjarne merged 1 commit into
dotnet:mainfrom
rolfbjarne:coreclr-coregraphics-cgpdfoperatortable

Conversation

@rolfbjarne

Copy link
Copy Markdown
Member
  • CoreCLR doesn't support generic Action delegates in reverse (P/Invokes), so
    we need to find a different solution.
  • The native CGPDFOperatorTable callback API is not very friendly to us,
    because we can't pass it callback-specific data, which means that the
    managed caller must conform to the FullAOT requirement of the managed
    callback (must be a static function; must have a MonoPInvokeCallback
    attribute).
  • We can leverage the new function pointer syntax in C# 9 to make these
    requirements enforced by the C# compiler (unmanaged function pointer +
    UnmanagedCallersOnly attribute). The resulting API is still clunky to use,
    but I don't see any way around that.

Fixes this monotouch-test failure with CoreCLR:

[FAIL] Tamarin : System.Runtime.InteropServices.MarshalDirectiveException : Cannot marshal 'parameter #3': Non-blittable generic types cannot be marshaled.
           at CoreGraphics.CGPDFOperatorTable.CGPDFOperatorTableSetCallback(IntPtr table, String name, Action`2 callback)
           at CoreGraphics.CGPDFOperatorTable.SetCallback(String name, Action`2 callback)
           at MonoTouchFixtures.CoreGraphics.PDFScannerTest.Tamarin() in xamarin-macios/tests/monotouch-test/CoreGraphics/PDFScannerTest.cs:line 102

Ref: dotnet/runtime#32963

…delegate.

* CoreCLR doesn't support generic Action delegates in reverse (P/Invokes), so
  we need to find a different solution.
* The native CGPDFOperatorTable callback API is not very friendly to us,
  because we can't pass it callback-specific data, which means that the
  managed caller must conform to the FullAOT requirement of the managed
  callback (must be a static function; must have a MonoPInvokeCallback
  attribute).
* We can leverage the new function pointer syntax in C# 9 to make these
  requirements enforced by the C# compiler (unmanaged function pointer +
  UnmanagedCallersOnly attribute). The resulting API is still clunky to use,
  but I don't see any way around that.

Fixes this monotouch-test failure with CoreCLR:

    [FAIL] Tamarin : System.Runtime.InteropServices.MarshalDirectiveException : Cannot marshal 'parameter #3': Non-blittable generic types cannot be marshaled.
               at CoreGraphics.CGPDFOperatorTable.CGPDFOperatorTableSetCallback(IntPtr table, String name, Action`2 callback)
               at CoreGraphics.CGPDFOperatorTable.SetCallback(String name, Action`2 callback)
               at MonoTouchFixtures.CoreGraphics.PDFScannerTest.Tamarin() in xamarin-macios/tests/monotouch-test/CoreGraphics/PDFScannerTest.cs:line 102

Ref: dotnet/runtime#32963
@rolfbjarne
rolfbjarne requested a review from spouliot as a code owner May 14, 2021 11:15
@rolfbjarne rolfbjarne added the not-notes-worthy Ignore for release notes label May 14, 2021

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍


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

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

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
Copy Markdown
Member

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!

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

API Diff (from PR only) (no change)
Generator Diff (no change)

🎉 All 82 tests passed 🎉

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 74d2220 into c1981b0

@mandel-macaque mandel-macaque added not-notes-worthy Ignore for release notes and removed not-notes-worthy Ignore for release notes labels May 14, 2021
@rolfbjarne
rolfbjarne merged commit e8f2672 into dotnet:main May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

not-notes-worthy Ignore for release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants