Change C# names of P/Invokes of objc_msgSend to be call-site specific. #47608

jkoritzinsky · 2021-01-29T00:35:37Z

As requested in #47592 (comment), I've renamed all instances of objc_msgSend P/Invokes to have call-site specific names since objc_msgSend has callsite-specific signatures.

ghost · 2021-01-29T00:35:46Z

Tagging subscribers to this area: @safern, @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

As requested in #47592 (comment), I've renamed all instances of objc_msgSend P/Invokes to have call-site specific names since objc_msgSend has callsite-specific signatures.

Author:	jkoritzinsky
Assignees:	-
Labels:	`area-System.Drawing`
Milestone:	-

src/libraries/Common/src/Interop/OSX/Interop.libobjc.cs

jkotas · 2021-01-29T02:25:16Z

src/libraries/System.Drawing.Common/src/System/Drawing/macFunctions.cs

+        [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend_stret")]
+        public static extern void get_bounds(ref Rect arect, IntPtr basePtr, IntPtr selector);
+        [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")]
+        public static extern bool get_isFlipped(IntPtr handle, IntPtr selector);


What is size of bool in Objective-C - does it match what the marshaler is going to use here?

It is not. This is a bug. I'll fix it tomorrow.

Is the "gist" linked from https://docs.microsoft.com/en-us/xamarin/ios/internals/objective-c-selectors buggy too then?

Yes, and it will cause failures on MacOS arm64.

jkotas · 2021-01-29T19:07:47Z

Also change src\libraries\common\src\Interop\OSX\Interop.libobjc.cs for consistency?

jkoritzinsky · 2021-01-29T21:35:19Z

Also change src\libraries\common\src\Interop\OSX\Interop.libobjc.cs for consistency?

Already done 👍🏻

jkotas · 2021-01-29T21:39:12Z

Ah, I have missed that it is part of this PR. get_operatingSystemVersion in that file is still on the old plan.

jkoritzinsky · 2021-01-29T21:41:28Z

We can’t really rename that one very well with the given model since we already have to customize the entry-point based on platform.

jkotas · 2021-01-29T21:48:48Z

src/libraries/System.Drawing.Common/src/System/Drawing/macFunctions.cs

        [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")]
+        public static extern IntPtr intptr_objc_msgSend(IntPtr basePtr, IntPtr selector);
+        [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend_stret")]
+        public static extern void void_objc_msgSend_stret_rrect(ref Rect arect, IntPtr basePtr, IntPtr selector);


Would this look better as rect_objc_msgSend_stret(out Rect arect ? The Xamarin examples are not 100% consistent - majority of them are using out, a few exception are using ref for some reason.

I don't think it particularly matters. We could also have the Rect type as the return value if we want.

jkotas · 2021-01-29T21:55:01Z

We can’t really rename that one very well with the given model since we already have to customize the entry-point based on platform.

Should the per-platform customization include change in the return value as well (void vs. NSOperatingSystemVersion)? Otherwise, if we are ok with using the return type assuming that the calling conventions will match, we can do it everywhere for consistency and to make the code a bit more readable.

Is there an established pattern that the Xamarin bindings use to deal with this difference between platforms?

jkoritzinsky · 2021-01-29T22:33:40Z

We can do return everywhere and it will work.

The Xamarin bindings (at least the ones on my VS4Mac instance) use out parameters for objc_msgSend_stret.

Which would you prefer that we do?

jkotas · 2021-01-30T01:02:51Z

I think we can

either stick to Xamarin conventions completely (e.g. use out to return)
grow our own modification of Xamarin conventions to make the code more pleasant to read (e.g. use regular return instead of out). If we grow our own modification to make the code more pleasant to read, we can do more than that. For example, we can also omit the argument names from the method name since they are not necessary to disambiguate.

Either one of these is fine with me.

jkoritzinsky · 2021-02-01T22:40:04Z

I decided to follow the Xamarin convention.

src/libraries/Common/src/Interop/OSX/Interop.libobjc.cs

src/libraries/System.Drawing.Common/src/System/Drawing/macFunctions.cs

into rename-msgsend

ghost · 2021-02-02T18:11:22Z

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Change C# names of P/Invokes of objc_msgSend to be call-site specific.

58faf1d

jkoritzinsky requested review from jkotas and AaronRobinsonMSFT January 29, 2021 00:35

dotnet-issue-labeler bot added the area-System.Drawing label Jan 29, 2021

jkoritzinsky mentioned this pull request Jan 29, 2021

Threadpool autorelease #47592

Merged

jkotas reviewed Jan 29, 2021

View reviewed changes

src/libraries/Common/src/Interop/OSX/Interop.libobjc.cs Outdated Show resolved Hide resolved

jkotas reviewed Jan 29, 2021

View reviewed changes

Rename to match suggested pattern

ed99c8f

jkotas reviewed Jan 29, 2021

View reviewed changes

Follow Xamarin convention in all objc_msgSend cases.

23d2093

jkotas reviewed Feb 2, 2021

View reviewed changes

src/libraries/Common/src/Interop/OSX/Interop.libobjc.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 2, 2021

View reviewed changes

src/libraries/System.Drawing.Common/src/System/Drawing/macFunctions.cs Outdated Show resolved Hide resolved

jkotas approved these changes Feb 2, 2021

View reviewed changes

AaronRobinsonMSFT approved these changes Feb 2, 2021

View reviewed changes

jkoritzinsky and others added 3 commits February 1, 2021 17:17

Fix typo

b336082

Rename to match convention.

97c9fc6

Merge branch 'rename-msgsend' of https://github.com/jkoritzinsky/runtime

2fbb23b

into rename-msgsend

jkoritzinsky added the auto-merge label Feb 2, 2021

jkotas approved these changes Feb 2, 2021

View reviewed changes

ghost merged commit 1562cf7 into dotnet:master Feb 2, 2021

ghost locked as resolved and limited conversation to collaborators Mar 4, 2021

jkoritzinsky deleted the rename-msgsend branch September 28, 2021 00:04

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change C# names of P/Invokes of objc_msgSend to be call-site specific. #47608

Change C# names of P/Invokes of objc_msgSend to be call-site specific. #47608

jkoritzinsky commented Jan 29, 2021

ghost commented Jan 29, 2021

jkotas Jan 29, 2021

jkoritzinsky Jan 29, 2021

jkotas Jan 29, 2021

jkoritzinsky Jan 29, 2021 •

edited

Loading

jkotas commented Jan 29, 2021

jkoritzinsky commented Jan 29, 2021

jkotas commented Jan 29, 2021

jkoritzinsky commented Jan 29, 2021

jkotas Jan 29, 2021 •

edited

Loading

jkoritzinsky Jan 29, 2021

jkotas commented Jan 29, 2021

jkoritzinsky commented Jan 29, 2021

jkotas commented Jan 30, 2021 •

edited

Loading

jkoritzinsky commented Feb 1, 2021

ghost commented Feb 2, 2021

Change C# names of P/Invokes of objc_msgSend to be call-site specific. #47608

Change C# names of P/Invokes of objc_msgSend to be call-site specific. #47608

Conversation

jkoritzinsky commented Jan 29, 2021

ghost commented Jan 29, 2021

jkotas Jan 29, 2021

Choose a reason for hiding this comment

jkoritzinsky Jan 29, 2021

Choose a reason for hiding this comment

jkotas Jan 29, 2021

Choose a reason for hiding this comment

jkoritzinsky Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

jkotas commented Jan 29, 2021

jkoritzinsky commented Jan 29, 2021

jkotas commented Jan 29, 2021

jkoritzinsky commented Jan 29, 2021

jkotas Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

jkoritzinsky Jan 29, 2021

Choose a reason for hiding this comment

jkotas commented Jan 29, 2021

jkoritzinsky commented Jan 29, 2021

jkotas commented Jan 30, 2021 • edited Loading

jkoritzinsky commented Feb 1, 2021

ghost commented Feb 2, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

jkoritzinsky Jan 29, 2021 •

edited

Loading

jkotas Jan 29, 2021 •

edited

Loading

jkotas commented Jan 30, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.