Closed Bug 1125947 Opened 5 years ago Closed 4 years ago

Fix -Wunreachable-code warning in netwerk/sctp/datachannel/DataChannel.cpp

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
Blocking Flags:
backlog tech-debt

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Wunreachable_DataChannel.patch (obsolete) — Splinter Review
Bug 807647 disabled the synchronous DataChannelConnection::SendPacket() code path by adding an `if (0)`, which causes this -Wunreachable-code warning:

netwerk/sctp/datachannel/DataChannel.cpp:717:11 [-Wunreachable-code] code will never be executed

SendPacket() is now only used asynchronously, so we can remove its always-true `bool release` parameter. We can also remove the `int res` error code because it is always 0.
Attachment #8554672 - Flags: review?(rjesup)
Comment on attachment 8554672 [details] [diff] [review]
Wunreachable_DataChannel.patch

Review of attachment 8554672 [details] [diff] [review]:
-----------------------------------------------------------------

r-, since I don't want to lose all the code and commentary (and never fix this or forget to push them to deal with it).  If we need this, let's convert into an #ifdef, or leave the comment (mostly) intact, and reference a new bug where the code to handle synchronous calls has been moved to.
Attachment #8554672 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #1)
> r-, since I don't want to lose all the code and commentary (and never fix
> this or forget to push them to deal with it).

Do we have a bug filed?


> If we need this, let's
> convert into an #ifdef, or leave the comment (mostly) intact, and reference
> a new bug where the code to handle synchronous calls has been moved to.

clang/gcc's -Wunreachable-code warning is not enabled by default, so fixing it is not critical.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I'd like to fix this DataChannel.cpp warning after all because I am trying to enable -Wunreachable-code warnings by default in mozilla-central.

Fix more clang warnings in netwerk/sctp/datachannel:

> netwerk/sctp/datachannel/DataChannel.cpp:727:11 [-Wunreachable-code] code will never be executed

Adding extra parens around the always-false conditional `0 /*peer->IsSTSThread()*/` suppresses the -Wunreachable-code warning about the SendPacket() never being reached.

> netwerk/sctp/datachannel/DataChannel.cpp:1835:19 [-Wshadow] declaration of 'i' shadows a previous local

This function has two for loops using two different index variables named `i`. Move the outer `i` inside the for loop scope to avoid the -Wshadow warning.

> netwerk/sctp/datachannel/DataChannel.h:531:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

Suppress clang's -Wimplicit-fallthrough warnings (not yet enabled by default in mozilla-central) about switch cases that fall through without a break or return statement. The warnings can be suppressed with the MOZ_FALLTHROUGH, a MFBT macro for a clang compiler annotation. MOZ_FALLTHROUGH is only needed on cases that have code:

switch (foo) {
  case 1: // These cases have no code. No fallthrough annotations are needed.
  case 2:
  case 3:
    foo = 4; // This case has code, so a fallthrough annotation is needed:
    MOZ_FALLTHROUGH;
  default:
    return foo;
}
Attachment #8554672 - Attachment is obsolete: true
Attachment #8681803 - Flags: review?(rjesup)
Attachment #8681803 - Flags: review?(rjesup) → review+
backlog: --- → tech-debt
https://hg.mozilla.org/mozilla-central/rev/ee5539ede95d
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.