Skip to content

Conversation

@filipnavara
Copy link
Member

Implement OperatingSystem.IsMacCatalyst and OperatingSystem.IsMacCatalystVersionAtLeast

Fixes #47768

Implement OperatingSystem.IsMacCatalyst and OperatingSystem.IsMacCatalystVersionAtLeast.
@ghost
Copy link

ghost commented Apr 9, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@akoeplinger akoeplinger requested a review from rolfbjarne April 9, 2021 12:20

const char* SystemNative_iOSSupportVersion()
{
@autoreleasepool
Copy link
Member

Choose a reason for hiding this comment

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

why do we need an autoreleasepool here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yeah I saw that, I was mostly wondering because we have some custom code in pal_autoreleasepool.m that does thread initialization stuff used by ThreadPool code in managed and I don't know if we need that here too:

void* SystemNative_CreateAutoreleasePool(void)
{
if (![NSThread isMultiThreaded])
{
// Start another no-op thread with the NSThread APIs to get NSThread into multithreaded mode.
// The NSAutoReleasePool APIs can't be used on secondary threads until NSThread is in multithreaded mode.
// See https://developer.apple.com/documentation/foundation/nsautoreleasepool for more information.
PlaceholderObject* placeholderObject = [[PlaceholderObject alloc] init];
// We need to use detachNewThreadSelector to put NSThread into multithreaded mode.
// We can't use detachNewThreadWithBlock since it doesn't change NSThread into multithreaded mode for some reason.
// See https://developer.apple.com/documentation/foundation/nswillbecomemultithreadednotification for more information.
[NSThread detachNewThreadSelector:@selector(noop:) toTarget:placeholderObject withObject:nil];
}
assert([NSThread isMultiThreaded]);
return [[NSAutoreleasePool alloc] init];
}

@rolfbjarne do you know the implications?

Copy link
Member Author

@filipnavara filipnavara Apr 9, 2021

Choose a reason for hiding this comment

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

Good point. I'm no expert on that but the documentation really suggests that it may be necessary... 🤷🏻‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine [1] if the new API are called from user+managed code.

If the dotnet runtime(s) calls (independently or before xamarin's runtime completed it's initialization) SystemNative_iOSSupportVersion then SystemNative_CreateAutoreleasePool should be called first (or to create the instance).

[1] if not it means we need to fix it

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'll modify it to call SystemNative_CreateAutoreleasePool instead of @autoreleasepool which should be safe. The code should not depend on Xamarin runtime being used/initialized (even if in most cases it will be).

Copy link
Member Author

Choose a reason for hiding this comment

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

The Apple documentation says that using NSAutoreleasePool manually would trip the ARC analyzer so I just extracted the code into helper method and kept using @autoreleasepool.

@filipnavara filipnavara marked this pull request as ready for review April 12, 2021 19:15
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Thank you!

@akoeplinger akoeplinger merged commit 5d0817a into dotnet:main Apr 13, 2021
@filipnavara filipnavara deleted the maccatalystversion branch April 13, 2021 12:19
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add IsMacCatalyst() and IsMacCatalystVersionAtLeast() to System.Runtime

4 participants