Closed Bug 1273965 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed
Blocking Flags:

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.)
I'm using clang 3.8 on 64-bit Ubuntu 16.04, BTW. (building current mozilla-inbound)
Attachment #8753958 - Flags: review?(docfaraday)
backlog: --- → webrtc/webaudio+
Rank: 35
Priority: -- → P3
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
Attachment #8753958 - Flags: review?(docfaraday)
Attachment #8753958 - Attachment is obsolete: true
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.
Attachment #8753966 - Flags: review?(docfaraday) → review+
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 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 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 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+
You need to log in before you can comment on or make changes to this bug.