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

Status: RESOLVED WORKSFORME

(Core :: Graphics: Text, defect)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[John Daggett (:jtd)]

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

Punting to Jonathan.

Comment 2

[Jonathan Kew [: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?

Comment 3

[Chris Peterson [:cpeterson]]

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

Comment 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

[Chris Peterson [:cpeterson]]

Looks good. The new graphite2 update fixed them all. :)