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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 1

[Daniel Holbert [:dholbert]]

I'm using clang 3.8 on 64-bit Ubuntu 16.04, BTW. (building current mozilla-inbound)

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: 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/

Comment 3

[Daniel Holbert [:dholbert]]

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.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: 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/

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: 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/

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: 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/

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: 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/

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Byron Campen [:bwc]]

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

[Byron Campen [:bwc]]

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.

Comment 11

[Byron Campen [:bwc]]

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

Comment 12

[Byron Campen [:bwc]]

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

Comment 13

[Daniel Holbert [:dholbert]]

Thanks!

Comment 14

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e788b61e7e83

Comment 15

[Pulsebot]

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

[Ryan VanderMeulen [:RyanVM]]

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