Closed Bug 1248513 Opened 4 years ago Closed 4 years ago
Fix static analysis errors in non-unified build for Audio
MozReview Request: Bug 1248513 - Fix static analysis errors in non-unified build for AudioStream.cpp. r=gerald.
58 bytes, text/x-review-board-request
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.
With unified build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e51fac16ec0d Without unified build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99d2cfc9eae
Assignee: nobody → jwwang
Review commit: https://reviewboard.mozilla.org/r/35419/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35419/
Attachment #8720666 - Flags: review?(gsquelart)
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!
You need to log in before you can comment on or make changes to this bug.