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

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 4

[JW Wang [:jwwang] [:jw_wang]]

With unified build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e51fac16ec0d
Without unified build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99d2cfc9eae

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1248513 - Fix static analysis errors in non-unified build for AudioStream.cpp. r=gerald.

Review commit: https://reviewboard.mozilla.org/r/35419/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35419/

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43cba2f5129c

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43cba2f5129c