-
Notifications
You must be signed in to change notification settings - Fork 522
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/CoreMidi] Use [UnmanagedCallersOnly] instead of [MonoPInvokeCallback] Partial Fix for #10470 #15774
Conversation
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.
Looks good otherwise!
src/CoreMidi/MidiServices.cs
Outdated
@@ -936,7 +960,7 @@ internal unsafe static IntPtr CreatePacketList (MidiPacket [] packets) | |||
[SupportedOSPlatform ("maccatalyst")] | |||
[SupportedOSPlatform ("macos")] | |||
#endif | |||
public class MidiPort : MidiObject { | |||
public unsafe class MidiPort : MidiObject { |
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.
In general we try to make the unsafe
scope as small as possible, so:
- Mark the P/Invoke as unsafe
- Add an
unsafe {}
block around the call to the P/Invoke.
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/CoreMidi/MidiServices.cs
Outdated
@@ -2095,7 +2146,7 @@ internal override void DisposeHandle () | |||
[SupportedOSPlatform ("maccatalyst")] | |||
[SupportedOSPlatform ("macos")] | |||
#endif | |||
public class MidiEndpoint : MidiObject { | |||
public unsafe class MidiEndpoint : MidiObject { |
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 here
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.
👍 After last Rolf's comment is addressed.
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 0 tests crashed, 41 tests failed, 4291 tests passed. Failures❌ bcl tests
Html Report (VSDrops) Download ❌ cecil tests
Html Report (VSDrops) Download ❌ dotnettests tests
Html Report (VSDrops) Download ❌ fsharp tests
Html Report (VSDrops) Download ❌ framework tests
Html Report (VSDrops) Download ❌ generator tests
Html Report (VSDrops) Download ❌ interdependent_binding_projects tests
Html Report (VSDrops) Download ❌ install_source tests
Html Report (VSDrops) Download ❌ introspection tests
Html Report (VSDrops) Download ❌ linker tests
Html Report (VSDrops) Download ❌ mac_binding_project tests
|
✅ 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 |
💻 [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) failed ❌Failed tests are:
Pipeline on Agent |
Ref: #10470.