Closed
Bug 1273965
Opened 6 years ago
Closed 6 years ago
MediaPipelineFactory.cpp:1015:10: warning: code will never be executed [-Wunreachable-code]
Categories
(Core :: WebRTC, defect, P3)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(4 files, 1 obsolete file)
Build warning: ========= media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:1015:10: warning: code will never be executed [-Wunreachable-code] do { NS_DebugBreak(NS_DEBUG_ASSERTION, "Shouldn't get here!", "Not Reached", "/scratch/work/builds/mozilla-inbound/mozilla/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp", 1015); MOZ_PretendNoReturn(); } while(0); ========= This is referring to the following line of code: > 1015 NS_NOTREACHED("Shouldn't get here!"); So: The compiler is agreeing with us -- this NS_NOTREACHED statement is unreachable! Hooray! :) But, the fact that it spams a warning is unfortunate. So, we should ideally restructure this code such that we can get rid of this NS_NOTREACHED. This comes after several nested "else-after-return" statements, which go against the coding style first guideline here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices I think if we collapsed those else-after-returns, it would likely remove the clause where this NS_NOTREACHED expression currently lives. (Note that this code lives in media/webrtc, but it doesn't seem to be upstream code -- it's code we added ourselves in bug 1091242.)
Assignee | ||
Comment 1•6 years ago
|
||
I'm using clang 3.8 on 64-bit Ubuntu 16.04, BTW. (building current mozilla-inbound)
Comment 2•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53606/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53606/
Updated•6 years ago
|
Attachment #8753958 -
Flags: review?(docfaraday)
Updated•6 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 35
Priority: -- → P3
Assignee | ||
Comment 3•6 years ago
|
||
Stealing, after checking with drno on IRC. (There are a bunch more else-after-returns that we can/should collapse while we're here -- and that allows us to drop the final NS_NOTREACHED as well.)
Assignee: nobody → dholbert
Assignee | ||
Updated•6 years ago
|
Attachment #8753958 -
Flags: review?(docfaraday)
Assignee | ||
Comment 4•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53612/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53612/
Attachment #8753966 -
Flags: review?(docfaraday)
Attachment #8753967 -
Flags: review?(docfaraday)
Attachment #8753968 -
Flags: review?(docfaraday)
Attachment #8753969 -
Flags: review?(docfaraday)
Assignee | ||
Comment 5•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53614/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53614/
Assignee | ||
Comment 6•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53616/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53616/
Assignee | ||
Comment 7•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53618/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53618/
Assignee | ||
Updated•6 years ago
|
Attachment #8753958 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
Note: my "part 2" here is pretty much the same as drno's original patch here. (and it addresses the build warning in comment 0) With that patch and "part 1" combined, "part 3" to naturally follows, to drop one other NS_NOTREACHED.
Updated•6 years ago
|
Attachment #8753966 -
Flags: review?(docfaraday) → review+
Comment 9•6 years ago
|
||
Comment on attachment 8753966 [details] MozReview Request: Bug 1273965 part 0: Drop "else" from some return-else-if statements in MediaPipelineFactory.cpp. r?bwc https://reviewboard.mozilla.org/r/53612/#review50356
Comment 10•6 years ago
|
||
Comment on attachment 8753967 [details] MozReview Request: Bug 1273965 part 1: Collapse some one-liner else-after-return patterns in MediaPipelineFactory.cpp. r?bwc https://reviewboard.mozilla.org/r/53614/#review50360 I tend to prefer handling the error case in the if block, but this is fine.
Attachment #8753967 -
Flags: review?(docfaraday) → review+
Comment 11•6 years ago
|
||
Comment on attachment 8753968 [details] MozReview Request: Bug 1273965 part 2: Collapse & de-indent one else-after-return in MediaPipelineFactory.cpp, and drop a now-clearly-unnecessary NS_NOTREACHED. r?bwc https://reviewboard.mozilla.org/r/53616/#review50362
Attachment #8753968 -
Flags: review?(docfaraday) → review+
Comment 12•6 years ago
|
||
Comment on attachment 8753969 [details] MozReview Request: Bug 1273965 part 3: Collapse & de-indent another else-after-return in MediaPipelineFactory.cpp, and drop a now-clearly-unnecessary NS_NOTREACHED. r?bwc https://reviewboard.mozilla.org/r/53618/#review50364
Attachment #8753969 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 13•6 years ago
|
||
Thanks!
Assignee | ||
Comment 14•6 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e788b61e7e83
Comment 15•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4d7167f19d https://hg.mozilla.org/integration/mozilla-inbound/rev/82a22f492f56 https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4a38912157 https://hg.mozilla.org/integration/mozilla-inbound/rev/e449e783210d
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f4d7167f19d https://hg.mozilla.org/mozilla-central/rev/82a22f492f56 https://hg.mozilla.org/mozilla-central/rev/fb4a38912157 https://hg.mozilla.org/mozilla-central/rev/e449e783210d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•