widget/cocoa/nsNativeThemeCocoa.mm:857:33 [-Wpointer-bool-conversion] address of array 'settings.minimumSizes' will always evaluate to 'true'

RESOLVED FIXED in Firefox 40

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8591261 [details] [diff] [review]
Wpointer-bool-conversion_minimumSizes.patch

Remove unnecessary null check of settings.minimumSizes array because, as clang warns, arrays are never null. Also add some bounds-checking asserts as a sanity check because sizeIndex is calculated through a couple different code paths and used to index into two different arrays, minimumSizes and naturalSizes.
Attachment #8591261 - Flags: review?(mstange)
Attachment #8591261 - Flags: review?(mstange) → review+
(Assignee)

Comment 1

3 years ago
Thanks! This was the only instance of -Wpointer-bool-conversion in mozilla-central. The -Wpointer-bool-conversion warning was added in Xcode's latest clang update.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82a14c4de34
https://hg.mozilla.org/mozilla-central/rev/e82a14c4de34
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 3

3 years ago
Created attachment 8592103 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa.patch

How embarrassing: my fix for the -Wpointer-bool-conversion warning introduced two -Wsign-compare warnings. :) I didn't catch this because the widget/cocoa directory is not FAIL_ON_WARNINGS yet.

sizeIndex and smallerControlSizeIndex are signed ints but my asserts compared them to size_t mozilla::ArrayLength(). This patch makes sizeIndex and smallerControlSizeIndex unsigned ints.

widget/cocoa/nsNativeThemeCocoa.mm:838:26 [-Wsign-compare] comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long')
widget/cocoa/nsNativeThemeCocoa.mm:858:24 [-Wsign-compare] comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long')
Attachment #8592103 - Flags: review?(mstange)
Comment on attachment 8592103 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa.patch

Since this variable is used to index into an array, size_t would be a better type to use. Can you also change the return type of EnumSizeForCocoaSize and change the other callers?
(Assignee)

Comment 5

3 years ago
Created attachment 8594047 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa-v2.patch

Changed EnumSizeForCocoaSize() and its callers to use size_t.
Attachment #8592103 - Attachment is obsolete: true
Attachment #8592103 - Flags: review?(mstange)
Attachment #8594047 - Flags: review?(mstange)
Comment on attachment 8594047 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa-v2.patch

This patch is empty.
Attachment #8594047 - Flags: review?(mstange)
(Assignee)

Comment 7

3 years ago
Created attachment 8594125 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa-v2.patch
Attachment #8594047 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Comment on attachment 8594125 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa-v2.patch

oops, sorry. Let's try this again.
Attachment #8594125 - Flags: review?(mstange)
Comment on attachment 8594125 [details] [diff] [review]
Wsign-compare_nsNativeThemeCocoa-v2.patch

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

I like!
Attachment #8594125 - Flags: review?(mstange) → review+
(Assignee)

Comment 10

3 years ago
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3aa154de9b1
https://hg.mozilla.org/mozilla-central/rev/c3aa154de9b1
You need to log in before you can comment on or make changes to this bug.