Suppress clang's -Wimplicit-fallthrough warnings in gfx/graphite2

RESOLVED WORKSFORME

Status

()

Core
Graphics: Text
RESOLVED WORKSFORME
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8675361 [details] [diff] [review]
suppress-graphite2-warnings.patch

Suppress clang's -Wimplicit-fallthrough warnings about switch cases that fall through without a break or return statement. The warnings can also be suppressed with a clang-specific C++11 annotation [[clang::fallthrough]] (aka MOZ_FALLTHROUGH macro from bug 1215411).

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; // aka [[clang::fallthrough]];
  default:
    return foo;
}

gfx/graphite2/src/Code.cpp:425:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/Code.cpp:508:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/Code.cpp:520:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/Code.cpp:534:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/Intervals.cpp:159:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/Intervals.cpp:166:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/inc/UtfCodec.h:135:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/inc/UtfCodec.h:136:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/graphite2/src/inc/UtfCodec.h:137:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
Attachment #8675361 - Flags: review?(jdaggett)

Comment 1

2 years ago
Comment on attachment 8675361 [details] [diff] [review]
suppress-graphite2-warnings.patch

Punting to Jonathan.
Attachment #8675361 - Flags: review?(jdaggett) → review?(jfkthame)
Martin, can we interest you in making graphite2 more clang-warning-clean? In particular, this is about the "implicit-fallthrough" warning (see above).

The fix would be to annotate these cases in the graphite2 source (I guess you'd need to #define and use a GR2_FALLTHROUGH macro, similar to our MOZ_FALLTHROUGH, as this is compiler-specific).

WDYT -- is this something you'd consider doing upstream?
Flags: needinfo?(martin_hosken)
(Assignee)

Comment 3

2 years ago
Note that clang's -Wimplicit-fallthrough warning is not enabled by default or -Wall and there is no equivalent gcc warning. However, I am trying to enable it for mozilla-central. So far I've found about five real bugs in Firefox code.

The good news is that all of the -Wimplicit-fallthrough warnings in graphite2 are intentional fallthrough cases documented with "no break" comments. :-)
I just landed a graphite2 update in bug 1220591. AFAIK, this should resolve these warnings, as it added GR_FALLTHROUGH annotations to a bunch of switch cases.
Flags: needinfo?(martin_hosken) → needinfo?(cpeterson)
(Assignee)

Comment 5

2 years ago
Looks good. The new graphite2 update fixed them all. :)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Depends on: 1220591
Flags: needinfo?(cpeterson)
Resolution: --- → WORKSFORME
(Assignee)

Updated

2 years ago
Attachment #8675361 - Flags: review?(jfkthame)
You need to log in before you can comment on or make changes to this bug.