1215894 - Suppress clang and MSVC warnings in gfx/harfbuzz

Status: RESOLVED WORKSFORME

(Core :: Graphics: Text, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: MOZ_FALLTHROUGH_harfbuzz.patch

Suppress clang's (optional) -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:
    [[clang::fallthrough]]; // or MOZ_FALLTHROUGH;
  default:
    return foo;
}

gfx/harfbuzz/src/hb-ot-shape-fallback.cc:229:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/harfbuzz/src/hb-ot-shape-fallback.cc:263:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/harfbuzz/src/hb-ot-shape-fallback.cc:283:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

And suppress MSVC x64 warnings about 32-bit shifts. I reported these MSVC warnings to the upstream harfbuzz repo:

https://github.com/behdad/harfbuzz/issues/146

gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(240) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(250) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(966) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(1518) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(1672) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(1819) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?).

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: 1215894_suppress-harfbuzz-warnings-v2.patch

Also suppress clang and gcc's -Wshadow warnings and gcc's -Wshadow-local and -Wshadow-compatible-local warnings. These -Wshadow* warnings are not enabled by default, but I am slowly fixing them in other bugs. Do you prefer that I fix these warnings upstream instead of suppressing them in mozilla-central?

gfx/harfbuzz/src/hb-open-type-private.hh:382:40 [-Wshadow] declaration shadows a field of 'OT::hb_serialize_context_t'
gfx/harfbuzz/src/hb-open-type-private.hh:478:31 [-Wshadow] declaration shadows a field of 'OT::hb_serialize_context_t'
gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh:361:48 [-Wshadow] declaration shadows a field of 'OT::hb_apply_context_t'
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1250:23 [-Wshadow] declaration shadows a local variable
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1625:26 [-Wshadow] declaration shadows a local variable
gfx/harfbuzz/src/hb-ot-shape-complex-myanmar.cc:459:23 [-Wshadow] declaration shadows a local variable
gfx/harfbuzz/src/hb-ot-shape-complex-use.cc:524:23 [-Wshadow] declaration shadows a local variable

gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1222:20 [-Wshadow-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1254:23 [-Wshadow-local] declaration of 'info' shadows a previous local
gfx/harfbuzz/src/hb-ot-shape-complex-myanmar.cc:426:20 [-Wshadow-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-myanmar.cc:458:23 [-Wshadow-local] declaration of 'info' shadows a previous local
gfx/harfbuzz/src/hb-ot-shape-complex-use.cc:483:20 [-Wshadow-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-use.cc:516:23 [-Wshadow-local] declaration of 'info' shadows a previous local

gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1595:23 [-Wshadow-compatible-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1634:26 [-Wshadow-compatible-local] declaration of 'i' shadows a previous local

Comment 2

[John Daggett (:jtd)]

Comment on attachment 8675444 [details] [diff] [review]
1215894_suppress-harfbuzz-warnings-v2.patch

Punting to Jonathan. My feeling is we should get these upstream but I'll defer to his opinion.

Comment 3

[Jonathan Kew [:jfkthame]]

Behdad, are these warnings that you're willing to fix upstream, or do we need to just suppress them locally?

Comment 4

[Behdad Esfahbod]

Yes, I'm upstreaming these now.  Can even get a release out today.

Comment 5

[Behdad Esfahbod]

These should all be fixed.  I'll roll a release soon, but would be nice if someone can test them.

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 8675444 [details] [diff] [review]
1215894_suppress-harfbuzz-warnings-v2.patch

Thanks, Behdad! I can test your changes soon. There is no rush to make a new release for these warnings. :)

Comment 7

[Behdad Esfahbod]

This should have been fixed by harfbuzz rolls since October.