Fix clang -Wstring-conversion warnings on OS X

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks 1 bug)

unspecified
mozilla30
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Attachment #8387435 - Flags: review?(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.
Attachment #8387435 - Flags: review?(bechen) → review?(giles)
Comment on attachment 8387435 [details] [diff] [review]
part-1-fix-webm.patch

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

Oops. Yay warnings!
Attachment #8387435 - Flags: review?(giles) → review+
Posted patch part-2-fix-layout.patch (obsolete) — Splinter Review
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().
Attachment #8388027 - Flags: review?(dbaron)
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.
Attachment #8388027 - Flags: review?(dbaron) → review-
(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.)
Point taken. :)

Replaced MOZ_ASSUME_UNREACHABLE("...") with MOZ_ASSERT(false, "...").
Attachment #8388027 - Attachment is obsolete: true
Attachment #8388036 - Flags: review?(dbaron)
Part 3: Fix clang -Wstring-conversion warnings in DOM UDPSocket code.

MOZ_ASSERT("...") will never fail because the char[] literal is always true.
Attachment #8388037 - Flags: review?(ekr)
Comment on attachment 8388036 [details] [diff] [review]
part-2-fix-layout-v2.patch

r=dbaron.  Thanks.
Attachment #8388036 - Flags: review?(dbaron) → review+
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.
Attachment #8388037 - Flags: review?(ekr) → review?(docfaraday)
Comment on attachment 8388037 [details] [diff] [review]
part-3-fix-UDPSocket.patch

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

Looks good to me.
bwc: did you forget to r+ the UDPSocket patch? <:)
Flags: needinfo?(docfaraday)
Attachment #8388037 - Flags: review?(docfaraday) → review+
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/18cf810962ab
https://hg.mozilla.org/mozilla-central/rev/d3ebc96a2080
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 1277428
You need to log in before you can comment on or make changes to this bug.