cocoabits / MASShortcut

Modern framework for managing global keyboard shortcuts compatible with Mac App Store. More details:
http://blog.shpakovski.com/2012/07/global-keyboard-shortcuts-in-cocoa.html
BSD 2-Clause "Simplified" License
1.52k stars 220 forks source link

Compiler warning for NSAccessibilityPriorityKey #76

Closed beaufour closed 8 years ago

beaufour commented 8 years ago

The only warning I get when compiling my project using MASShortcut is from MASShortcut:

/Users/user/repos/DisturbMeNot/Pods/MASShortcut/Framework/MASShortcutView.m:141:31: Comparison of address of 'NSAccessibilityPriorityKey' not equal to a null pointer is always true

Is this basically a check to see if the OS version is >= 10.9?

zoul commented 8 years ago

Yes, we use the symbol later on line 147 and since it’s not defined on older systems, that code would crash there, so we need to do an availability check.

I wonder why I am not getting that warning… I guess that’s again CocoaPods: since they pull the naked files into a different framework, the build settings may be different. The code is correct, so I guess we might simply disable the warning for the particular piece of code. I’m just not happy to introduce changes that are only needed because CocoaPods can’t simply take the framework as defined by our project, sigh.

Can you try this pragma please?

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wtautological-compare"
// Give VoiceOver users feedback on the result. Requires at least 10.9 to run.
if (_recording == NO && (&NSAccessibilityPriorityKey != NULL)) {
    NSString* msg = _shortcutValue ?
                     MASLocalizedString(@"Shortcut set", @"VoiceOver: Shortcut set") :
                     MASLocalizedString(@"Shortcut cleared", @"VoiceOver: Shortcut cleared");
    NSDictionary *announcementInfo = @{
        NSAccessibilityAnnouncementKey : msg,
        NSAccessibilityPriorityKey : @(NSAccessibilityPriorityHigh),
    };
    NSAccessibilityPostNotificationWithUserInfo(self, NSAccessibilityAnnouncementRequestedNotification, announcementInfo);
}
#pragma clang diagnostic pop
zoul commented 8 years ago

FYI, a1eeb57 is the crash fix with some detailed info.

beaufour commented 8 years ago

Thanks for the rapid answer!

I guess it is not strictly a CocoaPods thing. If I manually import your sources into my project and have -Wall enabled I'll probably get it too. The above pragma takes care of the warning. This is a bit of a more precise "cut" though:

-    if (_recording == NO && (&NSAccessibilityPriorityKey != NULL)) {
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wtautological-compare"
+    BOOL accessibilityAvailable = (&NSAccessibilityPriorityKey != NULL);
+#pragma clang diagnostic pop
+    if (_recording == NO && accessibilityAvailable) {
zoul commented 8 years ago

I consider the build process to be a private implementation detail of the framework that should be encapsulated from the consumer. That’s why I’m not very happy about anyone pulling the naked source files and compiling them by hand. The downsides should be obvious in the light of the two last issues: there’s simply too much to be broken. And that’s why I like Carthage better than CocoaPods, although I use both – and greatly admire the polish and the work that went into the CocoaPods infrastructure. Philosophical excursions aside :–), I’ll try to fix this as soon as possible, hopefully by tomorrow.

beaufour commented 8 years ago

I consider the build process to be a private implementation detail of the framework that should be encapsulated from the consumer. That’s why I’m not very happy about anyone pulling the naked source files and compiling them by hand. The downsides should be obvious in the light of the two last issues: there’s simply too much to be broken. And that’s why I like Carthage better than CocoaPods, although I use both – and greatly admire the polish and the work that went into the CocoaPods infrastructure.

That is a valid viewpoint, and also how most frameworks are shipped :) There is actually something nice about enforcing the same build warnings etc through all the source, but it is not exactly something I feel strongly about

Philosophical excursions aside :–), I’ll try to fix this as soon as possible, hopefully by tomorrow.

:smile: