Skip to content

[ClangImporter] Import NSNotificationName constants as nonisolated #71419

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

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Feb 6, 2024

These constants could be used with observer APIs from a different isolation
context, so it's more convenient to import them as nonisolated unless
they are explicitly isolated to a MainActor.

Resolves: rdar://114052705

@xedin
Copy link
Contributor Author

xedin commented Feb 6, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Feb 7, 2024

@swift-ci please test

These constants could be used with observer APIs from a different isolation
context, so it's more convenient to import them as `nonisolated` unless
they are explicitly isolated to a MainActor.

Resolves: rdar://114052705
Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

After rethinking what this PR is actually trying to accomplish, I think this is probably okay.

@@ -590,6 +590,13 @@ bool importer::isNSString(clang::QualType qt) {
return qt.getTypePtrOrNull() && isNSString(qt.getTypePtrOrNull());
}

bool importer::isNSNotificationName(clang::QualType type) {
if (auto *typealias = type->getAs<clang::TypedefType>()) {
return typealias->getDecl()->getName() == "NSNotificationName";
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a slight inconsistency with other parts of ClangImporter here—they only treat NSNotifcationName as special if it also (according to retrieveNewTypeAttr()) has a clang::SwiftNewTypeAttr attached. But I think this will only make a difference if the SDK has a broken declaration of NSNotificationName, so it's probably harmless.

@xedin
Copy link
Contributor Author

xedin commented Feb 7, 2024

@swift-ci please test

@xedin xedin merged commit 8bb6846 into swiftlang:main Feb 8, 2024
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.

2 participants