MediaPipelineFactory.cpp:1015:10: warning: code will never be executed [-Wunreachable-code]

RESOLVED FIXED in Firefox 49

Status

()

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

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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

2 years ago
I'm using clang 3.8 on 64-bit Ubuntu 16.04, BTW. (building current mozilla-inbound)
Created attachment 8753958 [details]
MozReview Request: Bug 1273965: refactor to remove NS_NOTREACHED

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

Updated

2 years ago
Attachment #8753958 - Flags: review?(docfaraday)

Updated

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 35
Priority: -- → P3
(Assignee)

Comment 3

2 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

2 years ago
Attachment #8753958 - Flags: review?(docfaraday)
(Assignee)

Comment 4

2 years ago
Created attachment 8753966 [details]
MozReview Request: Bug 1273965 part 0: Drop "else" from some return-else-if statements in MediaPipelineFactory.cpp. r?bwc

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

2 years ago
Created attachment 8753967 [details]
MozReview Request: Bug 1273965 part 1: Collapse some one-liner else-after-return patterns in MediaPipelineFactory.cpp. r?bwc

Review commit: https://reviewboard.mozilla.org/r/53614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53614/
(Assignee)

Comment 6

2 years ago
Created 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

Review commit: https://reviewboard.mozilla.org/r/53616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53616/
(Assignee)

Comment 7

2 years ago
Created 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

Review commit: https://reviewboard.mozilla.org/r/53618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53618/
(Assignee)

Updated

2 years ago
Attachment #8753958 - Attachment is obsolete: true
(Assignee)

Comment 8

2 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

2 years ago
Attachment #8753966 - Flags: review?(docfaraday) → review+

Comment 9

2 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

2 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

2 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

2 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

2 years ago
Thanks!

Comment 16

2 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
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.