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.

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

Support overloading Objective-C methods based on static/instance. #2607

Merged

Conversation

rolfbjarne
Copy link
Member

Two Objective-C methods can be named identically as long as one is static and
the other instance.

We must support this since Apple did just this (in the NSItemProviderReading /
NSItemProviderWriting protocols).

We solve it by prepending a '+' or '-' to the selector when hashing it (to
determine selector uniqueness, and to look the method up again at runtime).

Two Objective-C methods can be named identically as long as one is static and
the other instance.

We must support this since Apple did just this (in the NSItemProviderReading /
NSItemProviderWriting protocols).

We solve it by prepending a '+' or '-' to the selector when hashing it (to
determine selector uniqueness, and to look the method up again at runtime).
@monojenkins
Copy link
Collaborator

Build failure

[Preserve]
class OverloadByStaticity : NSObject
{
// Two methods/properties can
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: comment looks incomplete

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -235,8 +235,9 @@ public void Add (ObjCField field, ref List<Exception> exceptions)
{
// Check if there are any base types with the same property.
var base_type = BaseType;
var fieldNameInDictionary = (field.IsStatic ? "+" : "-") + field.Name;
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems to have the potential to create a lot of strings. Could we (later) try to:

  • lookup different static/instance collections ? or
  • only prepend "+" for static (and leave the instance without a sign) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose the implementation that I thought would have the lowest minimum memory requirements.

  • Having two different dictionaries, one for static and one for instance, would use more memory since there would be two dictionaries instead of one.
  • The strings are transient, the GC will collect them, and as such they don't contribute to the minimum memory requirements.
  • I could use a different key type (Dictionary<Tuple<string, bool>, ObjCMember for instance), but this means AOT-compiling a Dictionary with a struct as a key, which would increase code size (unless we already AOT-compile such a dictionary for something else).

I considered prepending + for static methods only, but then I thought of this (admittedly pathological) case:

[Export ("+foo")]
public void Foo () {}

which won't work with the static registrar, but works with the dynamic registrar (which is a use-case for Xamarin.Mac).

Finally I came to the conclusion I might be able to remove the whole problem when I make the static registrar do more at build time (see bug #56022).

@monojenkins
Copy link
Collaborator

Build success

@rolfbjarne rolfbjarne merged commit aa6004c into xamarin:xcode9 Sep 7, 2017
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Jul 26, 2022
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Jul 26, 2022
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Jul 26, 2022
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Jul 26, 2022
mandel-macaque added a commit that referenced this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants