-
Notifications
You must be signed in to change notification settings - Fork 114
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
Enable extension-safe API #144
base: master
Are you sure you want to change the base?
Conversation
I also removed some dead code that Xcode warned about and fixed a return value for the same reason. |
@@ -421,7 +421,7 @@ - (void)encodeWithCoder:(NSCoder *)aCoder | |||
+ (instancetype)new | |||
{ | |||
[self doesNotRecognizeSelector:_cmd]; | |||
return nil; | |||
return [super new]; |
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.
This change is undesirable since we don't want to instantiate any object here. An exception that leads to crash is desirable here since this is a programmer's error.
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.
Well it still does throw an exception, basically this line is dead code. But newer Xcode versions warn about returning nil
from new
, I thought it's better to fix this than to disable the warning in project settings, no?
What parts of API were you able to verify to work inside an extension? |
Linking the library to a framework instead of an app will also generate the warning. We were able to verify that an app can use the library just fine if it's not linking directly but inside a framework. The framework is using inside extensions that may use code to unarchive shortcuts from a persistent file, but we haven't tested that. |
Do I understand correctly that you do no use any of the SR API inside your extension? I'm trying to understand the use case so I can play with it. Current thoughts are that an additional extension-safe target could be added to SR with a subset of the API (e.g. without UI-related code and resources). |
"extension-safe API" refers to certain APIs that must not be called in extensions, most notably I don't see a reason to make a separate target. All the flag does is forbid certain APIs that are not used and communicates this to the linker. |
Alright, let me review relevant documentation. |
We've been using a custom fork with this flag set for a number of years for similar reasons (we have an extension-safe framework that includes ShortcutRecorder, but the actual use of ShortcutRecorder is within regular app processes). |
The framework could currently not be used in app extensions without generating a warning like this:
Since no non-extension-safe API is being used, enabling this flag clears the warning but requires no other changes.
Also enables the recommended
QUOTED_INCLUDE_IN_FRAMEWORK_HEADER
build setting, which also requires no other changes.