Fix -Wstring-conversion warnings in dom/media/compiledtest/TestAudioPacketizer.cpp

RESOLVED FIXED in Firefox 50

Status

()

Core
WebRTC: Audio/Video
P3
normal
Rank:
35
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
clang's -Wstring-conversion warning warns about the use of string literals as boolean expressions. This is not uncommon in "always fail" assertions, but is usually a bug in any other case. "Always fail" assertions are better expressed using MOZ_ASSERT_UNREACHABLE() or MOZ_CRASH().

dom/media/compiledtest/TestAudioPacketizer.cpp:48:10 [-Wstring-conversion] implicit conversion turns string literal into bool: 'const char [22]' to 'bool'
(Assignee)

Comment 1

2 years ago
Created attachment 8758366 [details] [diff] [review]
Bug NUMBER - Fix -Wstring-conversion warnings in dom/media/compiledtest/TestAudioPacketizer.cpp. r?
Attachment #8758366 - Flags: review?(padenot)

Updated

2 years ago
backlog: --- → tech-debt
Rank: 35
Priority: -- → P3
Comment on attachment 8758366 [details] [diff] [review]
Bug NUMBER - Fix -Wstring-conversion warnings in dom/media/compiledtest/TestAudioPacketizer.cpp. r?

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

This is an "always true" assertion, do you know how we should proceed to silence this ?

Maybe `assert(true && "message");` ?
Attachment #8758366 - Flags: review?(padenot)
(Assignee)

Comment 3

2 years ago
(In reply to Paul Adenot (:padenot) from comment #2)
> This is an "always true" assertion, do you know how we should proceed to
> silence this ?
> 
> Maybe `assert(true && "message");` ?

Assertion is always true and will never abort, so it is a no-op. The message will never be logged and AFAICT there's no need to keep it.
(Assignee)

Updated

2 years ago
Blocks: 1277428
I think it's important to keep it otherwise no test will have passed and the test framework will think it's an error. At least that was the case when I initially wrote the test.
(Assignee)

Comment 5

2 years ago
Created attachment 8760595 [details] [diff] [review]
MOZ_RELEASE_ASSERT_dom-media-compiledtest.patch

(In reply to Paul Adenot (:padenot) from comment #4)
> I think it's important to keep it otherwise no test will have passed and the
> test framework will think it's an error. At least that was the case when I
> initially wrote the test.

The assert(true) doesn't log anything or have any other side effect that can affect the outcome of the test. I believe these tests pass or fail depending on whether the test run crashes, asserts, or aborts.

Since these tests depend on asserts to fail, they don't actually test anything in release builds because the asserts are disabled. This new patch replaces the debug asserts with MOZ_RELEASE_ASSERT() which will check the condition and abort in both debug and release builds.
Attachment #8758366 - Attachment is obsolete: true
Attachment #8760595 - Flags: review?(padenot)
Comment on attachment 8760595 [details] [diff] [review]
MOZ_RELEASE_ASSERT_dom-media-compiledtest.patch

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

Thanks !
Attachment #8760595 - Flags: review?(padenot) → review+

Comment 7

2 years ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44208fb75c8b
Switch assert() to MOZ_RELEASE_ASSERT() in WebAudio compiled tests. r=padenot

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44208fb75c8b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.