980810 - Fix clang -Wstring-conversion warnings on OS X

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: part-1-fix-webm.patch

clang reports 15 -Wstring-conversion warnings. 8 are false positives, but 7 are real bugs in debug assertions.

Part 1: Fix clang -Wstring-conversion warnings in WebM container writer

content/media/webm/EbmlComposer.cpp:60:17 [-Wstring-conversion] implicit conversion turns string literal into bool: 'const char [35]' to 'bool'
content/media/webm/EbmlComposer.cpp:121:17 [-Wstring-conversion] implicit conversion turns string literal into bool: 'const char [35]' to 'bool'

These MOZ_ASSERT_IF calls will never assert because the first condition just determines whether the second condition, the actual assertion, is  a string constant, which is non-zero.

Comment 1

[Benjamin Chen [:bechen]]

Comment on attachment 8387435 [details] [diff] [review]
part-1-fix-webm.patch

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

Looks good, forward to Ralph.

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 8387435 [details] [diff] [review]
part-1-fix-webm.patch

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

Oops. Yay warnings!

Comment 3

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dde2a011bd9

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-2-fix-layout.patch

Part 2: Fix clang -Wstring-conversion warnings in layout.

MOZ_ASSERT("unexpected aType value") will never fail because the char[] literal is always true. MOZ_ASSUME_UNREACHABLE() will just log the message then abort().

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8388027 [details] [diff] [review]
part-2-fix-layout.patch

MOZ_ASSUME_UNREACHABLE is not an assertion; see http://hg.mozilla.org/mozilla-central/file/d01bf8596d3b/mfbt/Assertions.h#l381 .  This makes the code less safe (including in non-DEBUG builds) if the condition were ever to be false.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(While in most cases I'd normally review+ with conditions in a case like that, you've used one of the most dangerous footguns in the tree, and the minimal penalty for doing so is having to upload a new patch and ask for review again.)

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-2-fix-layout-v2.patch

Point taken. :)

Replaced MOZ_ASSUME_UNREACHABLE("...") with MOZ_ASSERT(false, "...").

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: part-3-fix-UDPSocket.patch

Part 3: Fix clang -Wstring-conversion warnings in DOM UDPSocket code.

MOZ_ASSERT("...") will never fail because the char[] literal is always true.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8388036 [details] [diff] [review]
part-2-fix-layout-v2.patch

r=dbaron.  Thanks.

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 8388037 [details] [diff] [review]
part-3-fix-UDPSocket.patch

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

Reassigning to bwc because I suspect this is his code and I'd like him to see it.

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8dde2a011bd9

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8388037 [details] [diff] [review]
part-3-fix-UDPSocket.patch

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

Looks good to me.

Comment 13

[Chris Peterson [:cpeterson]]

bwc: did you forget to r+ the UDPSocket patch? <:)

Comment 14

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18cf810962ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ebc96a2080

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18cf810962ab
https://hg.mozilla.org/mozilla-central/rev/d3ebc96a2080