Closed Bug 1313903 Opened 3 years ago Closed 3 years ago

Fix -Wmismatched-parameter-types warning in widget/cocoa/nsAppShell.mm

Categories

(Core :: Widget: Cocoa, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I get the following clang warning after upgrading to Xcode 8.1:

widget/cocoa/nsAppShell.mm:113:47 [-Wmismatched-parameter-types] conflicting parameter types in implementation of 'nextEventMatchingMask:untilDate:inMode:dequeue:': 'NSEventMask' (aka 'unsigned long long') vs 'NSUInteger' (aka 'unsigned long')

NSWindow's nextEventMatchingMask method expects a parameter of type NSEventMask, not NSUInteger:

https://developer.apple.com/reference/appkit/nswindow/1419304-nexteventmatchingmask
Attachment #8805826 - Flags: review?(spohl.mozilla.bugs)
Note that making this change (from NSUInteger to NSEventMask) introduces the reverse warning when compiling with older SDKs (used on the build machines). However, I still think we should move forward to NSEventMask, which is a more descriptive and future-proof type.

widget/cocoa/nsAppShell.mm:113:48: error: conflicting parameter types in implementation of 'nextEventMatchingMask:untilDate:inMode:dequeue:': 'NSUInteger' (aka 'unsigned long') vs 'NSEventMask' (aka 'unsigned long long') [-Werror,-Wmismatched-parameter-types]
- (NSEvent*)nextEventMatchingMask:(NSEventMask)mask

/Developer/SDKs/MacOSX10.7.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSApplication.h:202:48: note: previous definition is here
- (NSEvent *)nextEventMatchingMask:(NSUInteger)mask untilDate:(NSDate *)expiration inMode:(NSString *)mode dequeue:(BOOL)deqFlag;
It looks like they've switched parameter type in the 10.12 sdk (64-bit only). AFAICS all other sdk's uses NSUInteger.
Comment on attachment 8805826 [details] [diff] [review]
Wmismatched-parameter-types_nsAppShell.patch

Review of attachment 8805826 [details] [diff] [review]:
-----------------------------------------------------------------

Spoke with mstange and he agreed that ideally, we would support both the old and new type here.

::: widget/cocoa/nsAppShell.mm
@@ +109,5 @@
>    }
>    [super sendEvent:anEvent];
>  }
>  
> +- (NSEvent*)nextEventMatchingMask:(NSEventMask)mask

Could you #ifdef this such that we always use NSEventMask unless the SDK version is less than MAC_OS_X_VERSION_10_12, in which case we continue using NSUInteger?
Attachment #8805826 - Flags: review?(spohl.mozilla.bugs) → review-
(In reply to Stephen A Pohl [:spohl] from comment #3)
> Could you #ifdef this such that we always use NSEventMask unless the SDK
> version is less than MAC_OS_X_VERSION_10_12, in which case we continue using
> NSUInteger?

OK. That's a good suggestion. This patch adds #ifdefs for MAC_OS_X_VERSION_10_12 and __LP64__.

For posteriy, here is the #if __LP64__ used in 10.12's NSApplication.h:

#if __LP64__
- (nullable NSEvent *)nextEventMatchingMask:(NSEventMask)mask untilDate:(nullable NSDate *)expiration inMode:(NSRunLoopMode)mode dequeue:(BOOL)deqFlag;
…
#else
- (nullable NSEvent *)nextEventMatchingMask:(NSUInteger)mask untilDate:(nullable NSDate *)expiration inMode:(NSRunLoopMode)mode dequeue:(BOOL)deqFlag;
…
#endif
Attachment #8805826 - Attachment is obsolete: true
Attachment #8806914 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8806914 [details] [diff] [review]
Wmismatched-parameter-types-v2.patch

Review of attachment 8806914 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8806914 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4148e7b3d684
Fix -Wmismatched-parameter-types warning in widget/cocoa/nsAppShell.mm. r=spohl
https://hg.mozilla.org/mozilla-central/rev/4148e7b3d684
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.