Closed Bug 1248513 Opened 4 years ago Closed 4 years ago

Fix static analysis errors in non-unified build for AudioStream.cpp

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

Per bug 1248308 comment 1.

Try logs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc6bf100e95&selectedJob=16723667

/builds/slave/try-l64-st-an-d-00000000000000/build/src/dom/media/AudioStream.cpp:285:5: error: code will never be executed [-Werror,-Wunreachable-code]

/builds/slave/try-l64-st-an-d-00000000000000/build/src/dom/media/AudioStream.cpp:337:21: error: code will never be executed [-Werror,-Wunreachable-code]

revision:
https://hg.mozilla.org/try/rev/8de55cdac440
Bug 1248308 has landed, so at least I can continue the work that was blocked on it; Feel free to work on this one at your leisure.

Your attempt in bug 1248308 comment 9 might be the way to go, but it just seems like a lot of work to go around a compiler warning! And unfortunately it is much less readable than |if (A == B) ...|.
(Also you've used EnableIf with an extra struct, couldn't you have used template specialization?)

If instead there was a way to tell compilers that some code may intentionally be unreachable, it would be great. (After a quick search, I didn't find any.)

Interestingly, -Wunreachable was removed in gcc >3.4.4 because it was not stable depending on optimizations. :-)
https://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html
Maybe the unreachable check could be kept as a warning, not an error -- but that would probably be another bug addressed to our build overlords/overladies.
EnableIf is used to prevent code-gen for functions that won't be used at all which template specialization doesn't give you.

As far as readability is concerned, I can just go #ifdef MOZ_SAMPLE_TYPE_S16 without TMP tricks.
EnableIf: I see, good point, thank you.

Readability: Having looked at more of the code around the 'if', even though the change to templates looks significant, in the end it does separate the code more clearly for different situations. So I'm happy to retract my initial reluctance.
Comment on attachment 8720666 [details]
MozReview Request: Bug 1248513 - Fix static analysis errors in non-unified build for AudioStream.cpp. r=gerald.

https://reviewboard.mozilla.org/r/35419/#review32057

Thank you for taking care of this.
Attachment #8720666 - Flags: review?(gsquelart) → review+
Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/43cba2f5129c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.