Closed
Bug 1338925
Opened 7 years ago
Closed 7 years ago
Enable warnings-as-errors in widget/cocoa/
Categories
(Core :: Widget: Cocoa, defect)
Core
Widget: Cocoa
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: away, Assigned: away)
Details
Attachments
(4 files)
1.26 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.46 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
723 bytes,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
As far as I can tell, this is the last remaining non-third-party use of ALLOW_COMPILER_WARNINGS in our tree (if you count NSS and NSPR as third-party). Let's clean it up!
nsDragService.mm:223:31: error: expression result unused [-Werror,-Wunused-value] From a look at nearby code, here's my guess at what was intended.
Assignee: nobody → dmajor
Attachment #8836513 -
Flags: review?(mstange)
nsDeviceContextSpecX.mm:202:19: error: unused variable 'destPath' [-Werror,-Wunused-variable]
Attachment #8836514 -
Flags: review?(mstange)
nsChildView.mm:3642:12: error: 'NSView' may not respond to 'viewDidChangeBackingProperties' [-Werror] This is the patch I'm least sure about. Is it safe to just declare it among the undocumented methods? https://developer.apple.com/reference/appkit/nsview/1483742-viewdidchangebackingproperties says it's available since 10.7.
Attachment #8836515 -
Flags: review?(mstange)
Attachment #8836516 -
Flags: review?(mstange)
Comment 5•7 years ago
|
||
Thank you so much for doing this! I've been meaning to do it for a long time but never got around to it.
Comment 6•7 years ago
|
||
Comment on attachment 8836513 [details] [diff] [review] Fix the exception macro in nsDragService.mm Review of attachment 8836513 [details] [diff] [review]: ----------------------------------------------------------------- Oops, yes.
Attachment #8836513 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8836514 -
Flags: review?(mstange) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8836515 [details] [diff] [review] Declare viewDidChangeBackingProperties in nsChildView.h Review of attachment 8836515 [details] [diff] [review]: ----------------------------------------------------------------- Where did you see this warning? Locally, or on try? If locally, are you compiling with the 10.6 SDK or with something more recent? IIRC our build machines compile with the 10.9 SDK. But I suppose there could be more people out there building Gecko with the 10.6 SDK, -Werror would be breaking their builds without this patch, so we should land it anyway. Declaring it in this place is fine. Redeclarations of an existing method don't cause errors or other breakage. The only way this could go wrong is if other code called [someView viewDidChangeBackingProperties] on a pre-10.7 platform without checking whether the method exists (using respondsToSelector). Doing so would throw an Objective C exception at runtime and wouldn't give you any warnings at compile time. But we only call viewDidChangeBackingProperties if Cocoa already called this method so it must exist. And we only run on 10.9+ anyway.
Attachment #8836515 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8836516 -
Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #7) > Comment on attachment 8836515 [details] [diff] [review] > Declare viewDidChangeBackingProperties in nsChildView.h > > Review of attachment 8836515 [details] [diff] [review]: > ----------------------------------------------------------------- > > Where did you see this warning? Locally, or on try? If locally, are you > compiling with the 10.6 SDK or with something more recent? IIRC our build > machines compile with the 10.9 SDK. On try, which makes that build error particularly confusing.
Comment 9•7 years ago
|
||
NSView.h in the 10.7sdk has this line: - (void)viewDidChangeBackingProperties NS_AVAILABLE_MAC(10_7); // available in 10.7.4 So perhaps the try builders are using an older 10.7 version.
Comment 10•7 years ago
|
||
> So perhaps the try builders are using an older 10.7 version. which was suggested in bug 1296967, comment #4.
Comment 11•7 years ago
|
||
That would explain it. Thanks for checking!
Assignee | ||
Comment 12•7 years ago
|
||
Indeed: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/config/tooltool-manifests/macosx64/cross-releng.manifest#24
Comment 13•7 years ago
|
||
Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e06b22dfa60 Use the correct exception macro in nsDragService::IsValidType. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/81a31c193a1b Move debug-only variables to ifdef DEBUG. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe222eed019 Declare viewDidChangeBackingProperties on NSView. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2400aa8116 Enable warnings-as-errors in widget/cocoa/. r=mstange
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0e06b22dfa60 https://hg.mozilla.org/mozilla-central/rev/81a31c193a1b https://hg.mozilla.org/mozilla-central/rev/8fe222eed019 https://hg.mozilla.org/mozilla-central/rev/fb2400aa8116
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•